You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/30 22:51:20 UTC
[GitHub] [spark] mridulm opened a new pull request, #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
mridulm opened a new pull request, #39316:
URL: https://github.com/apache/spark/pull/39316
### What changes were proposed in this pull request?
Incorrect merge id is removed from the DB when a newer shuffle merge id is received.
### Why are the changes needed?
Bug fix
### Does this PR introduce _any_ user-facing change?
No, fixes a corner case bug
### How was this patch tested?
Unit test updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368517685
Thanks for the review @dongjoon-hyun, @wankunde, @zhouyejoe, @yabola :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368355942
Let me take a look, thanks @dongjoon-hyun.
Did not notice this failing in my local box ... weird.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wankunde commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
wankunde commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368151663
+1, LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368114664
+CC @otterc, @Ngone51
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368142176
Oh, could you take a look at the failures? It looks like relevant.
```
[info] *** 2 TESTS FAILED ***
[error] Failed tests:
[error] org.apache.spark.network.yarn.YarnShuffleServiceWithRocksDBBackendSuite
[error] org.apache.spark.network.yarn.YarnShuffleServiceWithLevelDBBackendSuite
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yabola commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
yabola commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368348865
@mridulm
I agree with you. The failed UT is as expected and we should modify the UT.
My previous PR also do it https://github.com/apache/spark/pull/38560/files#r1031350719
Actually in my previous PR, I tried unify this deletion logic: delete `mergePartitionsInfo.shuffleMergePartitions` at the same time deleting the DB.
What more, I also want to unify the deleting the finalized partition information ( if higher mergeId come and merge partitions was finalized, we should clean finalized partitions, please see here https://github.com/apache/spark/pull/38560/files#diff-d544fbb952b61283b3d18ca10a5027528efc4f2f65047130da015ae7754c117fR502)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368517077
Got it. Thank you, @mridulm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368517113
Merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhouyejoe commented on a diff in pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
zhouyejoe commented on code in PR #39316:
URL: https://github.com/apache/spark/pull/39316#discussion_r1059675164
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -227,15 +227,15 @@ AppShufflePartitionInfo getOrCreateAppShufflePartitionInfo(
// Higher shuffleMergeId seen for the shuffle ID meaning new stage attempt is being
// run for the shuffle ID. Close and clean up old shuffleMergeId files,
// happens in the indeterminate stage retries
- AppAttemptShuffleMergeId appAttemptShuffleMergeId =
- new AppAttemptShuffleMergeId(
- appShuffleInfo.appId, appShuffleInfo.attemptId, shuffleId, shuffleMergeId);
+ AppAttemptShuffleMergeId currrentAppAttemptShuffleMergeId =
+ new AppAttemptShuffleMergeId(appShuffleInfo.appId, appShuffleInfo.attemptId,
+ shuffleId, latestShuffleMergeId);
logger.info("{}: creating a new shuffle merge metadata since received " +
- "shuffleMergeId is higher than latest shuffleMergeId {}",
- appAttemptShuffleMergeId, latestShuffleMergeId);
+ "shuffleMergeId {} is higher than latest shuffleMergeId {}",
+ currrentAppAttemptShuffleMergeId, shuffleMergeId, latestShuffleMergeId);
submitCleanupTask(() ->
- closeAndDeleteOutdatedPartitions(
- appAttemptShuffleMergeId, mergePartitionsInfo.shuffleMergePartitions));
+ closeAndDeleteOutdatedPartitions(currrentAppAttemptShuffleMergeId,
+ mergePartitionsInfo.shuffleMergePartitions));
Review Comment:
Nit: 4 spaces for continued lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368356980
Agree with @yabola, the test is broken: I will fix the test to the expected output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368115059
+CC @wankunde, this fixes the bug I had [described earlier](https://github.com/apache/spark/pull/37922#discussion_r990753031).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39316:
URL: https://github.com/apache/spark/pull/39316#issuecomment-1368516939
@dongjoon-hyun:
> According to the JIRA, is this for both Apache Spark 3.3.2 and 3.4.0, @mridulm ?
This is only for master, I have updated the jira ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm closed pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
Posted by GitBox <gi...@apache.org>.
mridulm closed pull request #39316: [SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received
URL: https://github.com/apache/spark/pull/39316
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org