[GitHub] [spark] otterc commented on a diff in pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager

[GitBox <gi...@apache.org>]

otterc commented on code in PR #37052:
URL: https://github.com/apache/spark/pull/37052#discussion_r914938651


##########
core/src/main/scala/org/apache/spark/storage/BlockManager.scala:
##########
@@ -526,16 +544,8 @@ private[spark] class BlockManager(
 
     blockManagerId = if (idFromMaster != null) idFromMaster else id
 
-    shuffleServerId = if (externalShuffleServiceEnabled) {
-      logInfo(s"external shuffle service port = $externalShuffleServicePort")
-      BlockManagerId(executorId, blockTransferService.hostName, externalShuffleServicePort)
-    } else {
-      blockManagerId
-    }

Review Comment:
   When ESS is enabled, `shuffleServiceId` should be assigned before calling `registerWithExternalShuffleServer()` because that method uses it.
   When ESS is not enabled, then `shuffleServiceId` is the id with topology and I didn't want to change that.



-- 
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