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/07/01 18:54:44 UTC
[GitHub] [spark] otterc opened a new pull request, #37052: SPARK-39647 Register the executor with ESS before registering the BlockManager
otterc opened a new pull request, #37052:
URL: https://github.com/apache/spark/pull/37052
### What changes were proposed in this pull request?
Currently the executors register with the ESS after the `BlockManager` registration with the `BlockManagerMaster`. This order creates a problem with the push-based shuffle. A registered BlockManager node is picked up by the driver as a merger but the shuffle service on that node is not yet ready to merge the data which causes block pushes to fail until the local executor registers with it. This fix is to reverse the order, that is, register with the ESS before registering the `BlockManager`
### Why are the changes needed?
They are needed to fix the issue which causes block pushes to fail.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added a UT.
--
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] attilapiros commented on a diff in pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on code in PR #37052:
URL: https://github.com/apache/spark/pull/37052#discussion_r914058583
##########
core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala:
##########
@@ -2177,6 +2177,43 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
assert(kryoException.getMessage === "java.io.IOException: Input/output error")
}
+ test("SPARK-39647: Failure to register with ESS should prevent registering the BM") {
+ val timeoutExec = "timeoutExec"
Review Comment:
nit: this val is not needed anymore.
--
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 #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1181326481
Merging to master and branch-3.3
--
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] otterc commented on pull request #37052: SPARK-39647 Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
otterc commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1172629436
@mridulm @zhouyejoe @Ngone51 Please help review
--
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 #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
mridulm closed pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
URL: https://github.com/apache/spark/pull/37052
--
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] otterc commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
otterc commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1177035357
> @otterc GA failed
This below test failed. I don't think it is related to this change.
```
info] *** 1 TEST FAILED ***
[error] Failed: Total 3385, Failed 1, Errors 0, Passed 3384, Ignored 9, Canceled 2
[error] Failed tests:
[error] org.apache.spark.scheduler.HealthTrackerIntegrationSuite
```
--
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 #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1181327891
Merged to master and branch-3.3
Thanks for fixing this @otterc !
Thanks for reviewing @attilapiros, @zhouyejoe, @Ngone51 and @weixiuli :-)
--
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] otterc commented on a diff in pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by 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
[GitHub] [spark] mridulm commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1173371065
Took a quick look, the change looks good to me.
+CC @attilapiros, since you had touched this codepath reasonably recently.
--
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 pull request #37052: SPARK-39647 Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
zhouyejoe commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1172648378
The fix is neat. Overall 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 #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1182527170
Good question @dongjoon-hyun, I was not sure about 3.2 actually - I was assuming it wont be relevant there (since this change is relevant mostly in context of push based shuffle). But after looking at an internal PR by @otterc, I believe you are right - we should merge it to 3.2 as well.
--
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] Ngone51 commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1177105984
I have retrigerred the jenkins job..
--
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] Ngone51 commented on a diff in pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #37052:
URL: https://github.com/apache/spark/pull/37052#discussion_r914491973
##########
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:
This could be remained unchanged? If so, I think it's better to keep `shuffleServerId` assignment in one place.
--
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] otterc commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
otterc commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1177877890
The same test in HealthTrackerIntegrationSuite failed again
```
Bad node with multiple executors, job will still succeed with the right confs *** FAILED *** (20 seconds, 47 milliseconds)
[info] java.util.concurrent.TimeoutException: Futures timed out after [20 seconds]
[info] at scala.concurrent.impl.Promise$DefaultPromise.ready(Promise.scala:259)
[info] at scala.concurrent.impl.Promise$DefaultPromise.ready(Promise.scala:187)
[info] at org.apache.spark.util.ThreadUtils$.awaitReady(ThreadUtils.scala:334)
[info] at org.apache.spark.scheduler.SchedulerIntegrationSuite.awaitJobTermination(SchedulerIntegrationSuite.scala:276)
[info] at org.apache.spark.scheduler.HealthTrackerIntegrationSuite.$anonfun$new$9(HealthTrackerIntegrationSuite.scala:92)
```
It seems unrelated to this change
--
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 #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1186029561
Now that 3.2.2 vote has passed, can you create a PR against branch-3.2 for this change @otterc ? Thx
--
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] attilapiros commented on a diff in pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on code in PR #37052:
URL: https://github.com/apache/spark/pull/37052#discussion_r914027452
##########
core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala:
##########
@@ -2177,6 +2177,46 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
assert(kryoException.getMessage === "java.io.IOException: Input/output error")
}
+ test("SPARK-39647: Failure to register with ESS should prevent registering the BM") {
+ val timeoutExec = "timeoutExec"
+ val handler = new NoOpRpcHandler {
+ override def receive(
+ client: TransportClient,
+ message: ByteBuffer,
+ callback: RpcResponseCallback): Unit = {
+ val msgObj = BlockTransferMessage.Decoder.fromByteBuffer(message)
+ msgObj match {
+
+ case exec: RegisterExecutor if exec.execId == timeoutExec =>
Review Comment:
As you are using this handler for only one executor ("timeoutExec") you can remove the if part.
--
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] attilapiros commented on a diff in pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
attilapiros commented on code in PR #37052:
URL: https://github.com/apache/spark/pull/37052#discussion_r914025635
##########
core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala:
##########
@@ -2177,6 +2177,46 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
assert(kryoException.getMessage === "java.io.IOException: Input/output error")
}
+ test("SPARK-39647: Failure to register with ESS should prevent registering the BM") {
+ val timeoutExec = "timeoutExec"
+ val handler = new NoOpRpcHandler {
+ override def receive(
+ client: TransportClient,
+ message: ByteBuffer,
+ callback: RpcResponseCallback): Unit = {
+ val msgObj = BlockTransferMessage.Decoder.fromByteBuffer(message)
+ msgObj match {
+
+ case exec: RegisterExecutor if exec.execId == timeoutExec =>
+ () // No reply to generate client-side timeout
+ }
+ }
+ }
+ val transConf = SparkTransportConf.fromSparkConf(conf, "shuffle", numUsableCores = 0)
+ Utils.tryWithResource(new TransportContext(transConf, handler, true)) { transCtx =>
+ // a server which delays response 50ms and must try twice for success.
Review Comment:
This comment is copy-pasted from "SPARK-20640: Shuffle registration timeout and maxAttempts conf are working" but here it is not a valid statement.
--
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 #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1182182128
BTW, do you think we can have this at `branch-3.2` ?
--
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] Ngone51 commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1176943576
@otterc GA failed
--
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] Ngone51 commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1177643483
@otterc there's still failed tests
--
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] Ngone51 commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1177942679
@otterc Could you push an empty commit to retrigger the tests?
--
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] AmplabJenkins commented on pull request #37052: SPARK-39647 Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1173032744
Can one of the admins verify this patch?
--
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] otterc commented on pull request #37052: [SPARK-39647][CORE] Register the executor with ESS before registering the BlockManager
Posted by GitBox <gi...@apache.org>.
otterc commented on PR #37052:
URL: https://github.com/apache/spark/pull/37052#issuecomment-1180723189
All the tests pass now @Ngone51 @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