mridulm commented on code in PR #37922: URL: https://github.com/apache/spark/pull/37922#discussion_r1067973800
########## core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala: ########## @@ -321,6 +321,12 @@ class BlockManagerMasterEndpoint( } private def removeShuffle(shuffleId: Int): Future[Seq[Boolean]] = { + val mergerLocations = + if (Utils.isPushBasedShuffleEnabled(conf, isDriver)) { + mapOutputTracker.getShufflePushMergerLocations(shuffleId) + } else { + Seq.empty[BlockManagerId] + } Review Comment: Thinking more, I think your assessment is right - since MOT is non-null : will need to check more. If right, we need to move `removeShuffleFromExecutorsFutures` to the end of the method (that should be sufficient) ... currently it is a race condition, and bug need not get triggered always The test above is slightly broken - the initial initialization is not getting handled (when driver would be registered). A bit late for me to try to fix it :-) Can you take a look at it @wankunde ? We can fix the test and add to the PR ? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org