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 2020/06/24 15:19:23 UTC
[GitHub] [spark] Ngone51 opened a new pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Ngone51 opened a new pull request #28924:
URL: https://github.com/apache/spark/pull/28924
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR adds the check to see whether the executor is lost (by asking the `CoarseGrainedSchedulerBackend`) after timeout error raised in `BlockManagerMasterEndponit` due to removing blocks(e.g. RDD, broadcast, shuffle). If the executor is lost, we will ignore the error. Otherwise, throw the error.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
When removing blocks(e.g. RDD, broadcast, shuffle), `BlockManagerMaserEndpoint` will make RPC calls to each known `BlockManagerSlaveEndpoint` to remove the specific blocks. The PRC call sometimes could end in a timeout when the executor has been lost, but only notified the `BlockManagerMasterEndpoint` after the removing call has already happened. The timeout error could therefore fail the whole job.
In this case, we actually could just ignore the error since those blocks on the lost executor could be considered as removed already.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Added unit 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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654650587
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r451922315
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -168,6 +177,37 @@ class BlockManagerMasterEndpoint(
stop()
}
+ private def handleFailure[T](
Review comment:
I see, thanks.
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652981089
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r447580528
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,12 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no actual timeout property controls it.
+ * And timeout property should never be accessed since infinite means we never timeout.
Review comment:
Updated.
`timeout property` actually is the third field of `RpcTimeout`. You could check its usage inside `RpcTimeout`.
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452076921
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala
##########
@@ -153,8 +154,8 @@ class BlockManagerMaster(
logWarning(s"Failed to remove shuffle $shuffleId - ${e.getMessage}", e)
)(ThreadUtils.sameThread)
if (blocking) {
- timeout.awaitResult(future)
- }
+ // the underlying Futures will timeout anyway, so it's safe to use infinite here
+ infinite.awaitResult(future) }
Review comment:
ditto
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-651710437
ping @tgravescs @jiangxb1987 Could you also take a look? thanks!
----------------------------------------------------------------
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.
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] tgravescs commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654346002
test this please
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650811453
Retest this please.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652980402
**[Test build #124903 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124903/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648891974
**[Test build #124490 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124490/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` case class IsExecutorAlive(executorId: String) extends CoarseGrainedClusterMessage`
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r448945419
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,13 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no actual timeout property controls it.
Review comment:
yea, sounds better.
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656519685
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648890455
**[Test build #124490 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124490/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
----------------------------------------------------------------
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.
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] prakharjain09 commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
prakharjain09 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r448190015
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -168,6 +173,37 @@ class BlockManagerMasterEndpoint(
stop()
}
+ private def handleFailure[T](
+ blockType: String,
+ blockId: String,
+ bmId: BlockManagerId,
+ defaultValue: T): PartialFunction[Throwable, T] = {
Review comment:
[nit] since this method deals with handling block removal failures explicitly, can we rename it to reflect the same. maybe something like: handleBlockRemovalFailure?
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452164478
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -177,6 +181,95 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
blockManager.stop()
}
+ private def setupBlockManagerMasterWithBlocks(withLost: Boolean): Unit = {
+ // set up a simple DriverEndpoint which simply adds executorIds and
+ // checks whether a certain executorId has been added before.
+ val driverEndpoint = rpcEnv.setupEndpoint(CoarseGrainedSchedulerBackend.ENDPOINT_NAME,
+ new RpcEndpoint {
+ private val executorSet = mutable.HashSet[String]()
+ override val rpcEnv: RpcEnv = this.rpcEnv
+ override def receiveAndReply(context: RpcCallContext): PartialFunction[Any, Unit] = {
+ case CoarseGrainedClusterMessages.RegisterExecutor(executorId, _, _, _, _, _, _, _) =>
+ executorSet += executorId
+ context.reply(true)
+ case CoarseGrainedClusterMessages.IsExecutorAlive(executorId) =>
+ context.reply(executorSet.contains(executorId))
+ }
+ }
+ )
+
+ def createAndRegisterBlockManager(timeout: Boolean): BlockManagerId = {
+ val id = if (timeout) "timeout" else "normal"
+ val bmRef = rpcEnv.setupEndpoint(s"bm-$id", new RpcEndpoint {
+ override val rpcEnv: RpcEnv = this.rpcEnv
+ private def reply[T](context: RpcCallContext, response: T): Unit = {
+ if (timeout) {
+ Thread.sleep(conf.getTimeAsMs(Network.RPC_ASK_TIMEOUT.key) + 1000)
+ }
+ context.reply(response)
+ }
+
+ override def receiveAndReply(context: RpcCallContext): PartialFunction[Any, Unit] = {
+ case RemoveRdd(_) => reply(context, 1)
+ case RemoveBroadcast(_, _) => reply(context, 1)
+ case RemoveShuffle(_) => reply(context, true)
+ }
+ })
+ val bmId = BlockManagerId(s"exec-$id", "localhost", 1234, None)
+ master.registerBlockManager(bmId, Array.empty, 2000, 0, bmRef)
+ }
+
+ // set up normal bm1
+ val bm1Id = createAndRegisterBlockManager(false)
+ // set up bm2, which intentionally takes more time than RPC_ASK_TIMEOUT to
+ // remove rdd/broadcast/shuffle in order to raise timeout error
+ val bm2Id = createAndRegisterBlockManager(true)
+
+ driverEndpoint.askSync[Boolean](CoarseGrainedClusterMessages.RegisterExecutor(
+ bm1Id.executorId, null, bm1Id.host, 1, Map.empty, Map.empty,
+ Map.empty, 0))
+
+ if (!withLost) {
Review comment:
what does `withLost` means?
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650811557
**[Test build #124606 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124606/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652074356
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124660/
Test 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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654631366
**[Test build #125169 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125169/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650811557
**[Test build #124606 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124606/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656636315
**[Test build #125568 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125568/testReport)** for PR 28924 at commit [`828aa2b`](https://github.com/apache/spark/commit/828aa2bbdbf097e8fde22c42d780673852537efb).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654653686
**[Test build #125186 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125186/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654348285
**[Test build #125103 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125103/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648890455
**[Test build #124490 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124490/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656533667
**[Test build #125578 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125578/testReport)** for PR 28924 at commit [`319d925`](https://github.com/apache/spark/commit/319d9257b200bb087afe15aace8b66d96925ce93).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654466079
**[Test build #125103 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125103/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452076697
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala
##########
@@ -142,8 +143,8 @@ class BlockManagerMaster(
logWarning(s"Failed to remove RDD $rddId - ${e.getMessage}", e)
)(ThreadUtils.sameThread)
if (blocking) {
- timeout.awaitResult(future)
- }
+ // the underlying Futures will timeout anyway, so it's safe to use infinite here
+ infinite.awaitResult(future) }
Review comment:
nit: code styl
```
if (blocking) {
infinite.awaitResult(future)
}
```
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala
##########
@@ -142,8 +143,8 @@ class BlockManagerMaster(
logWarning(s"Failed to remove RDD $rddId - ${e.getMessage}", e)
)(ThreadUtils.sameThread)
if (blocking) {
- timeout.awaitResult(future)
- }
+ // the underlying Futures will timeout anyway, so it's safe to use infinite here
+ infinite.awaitResult(future) }
Review comment:
nit: code style
```
if (blocking) {
infinite.awaitResult(future)
}
```
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654648147
retest this please
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r447579643
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -93,6 +94,7 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
.set(MEMORY_STORAGE_FRACTION, 0.999)
.set(Kryo.KRYO_SERIALIZER_BUFFER_SIZE.key, "1m")
.set(STORAGE_UNROLL_MEMORY_THRESHOLD, 512L)
+ .set(Network.RPC_ASK_TIMEOUT, "5s")
Review comment:
In the newly added tests, we need to simulate the timeout error from BlockManager. But at the same time, we also don't want the test run too long since the default timeout value is 120s. Therefore, we choose a quite short timeout for the tests. On the other hand, we don't set it to a smaller value, e.g. 1s, which may cause test flaky.
Note that the best way to set the timeout value is to set it for the newly added tests locally instead of setting it globally. However, with the limitation of the current test framework in Core side, it's hard to set it locally since it requires more changes.
----------------------------------------------------------------
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.
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] prakharjain09 commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
prakharjain09 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r448189214
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -235,7 +269,10 @@ class BlockManagerMasterEndpoint(
val removeMsg = RemoveShuffle(shuffleId)
Future.sequence(
blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Boolean](removeMsg)
+ bm.slaveEndpoint.ask[Boolean](removeMsg).recover {
+ // use false as default value means no shuffle data were removed
+ handleFailure("shuffle", shuffleId.toString, bm.blockManagerId, false)
Review comment:
Previously we were not swallowing IOException here for removing shuffle blocks? But handleFailure method will start swallowing it? is this intentional?
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452662540
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -177,6 +181,95 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
blockManager.stop()
}
+ private def setupBlockManagerMasterWithBlocks(withLost: Boolean): Unit = {
Review comment:
ok sure.
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652074346
Merged build finished. Test 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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-653097209
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124903/
Test 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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650811796
**[Test build #124606 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124606/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
* This patch **fails build dependency tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` case class IsExecutorAlive(executorId: String) extends CoarseGrainedClusterMessage`
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654348675
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-651710234
@holdenk Thanks for review. I've also updated the PR description for the user-facing 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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648891994
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650949521
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r447555905
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -168,6 +177,37 @@ class BlockManagerMasterEndpoint(
stop()
}
+ private def handleFailure[T](
Review comment:
What's `docstring`? Could you explain more?
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654788834
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654650587
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452076497
##########
File path: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedClusterMessage.scala
##########
@@ -132,4 +132,6 @@ private[spark] object CoarseGrainedClusterMessages {
// Used internally by executors to shut themselves down.
case object Shutdown extends CoarseGrainedClusterMessage
+ // The message to check if scheduler thinks the executor is alive or not.
Review comment:
can we be more specific about the "scheduler" here? which scheduler?
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652073453
**[Test build #124660 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124660/testReport)** for PR 28924 at commit [`5a02ef2`](https://github.com/apache/spark/commit/5a02ef2f013c816165e97f3c9ec0eded2a2d9a03).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] holdenk commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r447303794
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -95,6 +97,13 @@ class BlockManagerMasterEndpoint(
private val externalShuffleServiceRddFetchEnabled: Boolean = externalBlockStoreClient.isDefined
private val externalShuffleServicePort: Int = StorageUtils.externalShuffleServicePort(conf)
+ private lazy val driverEndpoint = {
Review comment:
Would `makeDriverRef` in `RpcUtils` be appropriate here?
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -168,6 +177,37 @@ class BlockManagerMasterEndpoint(
stop()
}
+ private def handleFailure[T](
Review comment:
I think this function could use a docstring.
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,12 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no actual timeout property controls it.
+ * And timeout property should never be accessed since infinite means we never timeout.
Review comment:
I'm not sure I follow this sentence correctly, can you try and reword it?
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -350,11 +388,13 @@ class BlockManagerMasterEndpoint(
if (locations != null) {
locations.foreach { blockManagerId: BlockManagerId =>
val blockManager = blockManagerInfo.get(blockManagerId)
- if (blockManager.isDefined) {
+ blockManager.foreach { bm =>
// Remove the block from the slave's BlockManager.
// Doesn't actually wait for a confirmation and the message might get lost.
// If message loss becomes frequent, we should add retry logic here.
- blockManager.get.slaveEndpoint.ask[Boolean](RemoveBlock(blockId))
+ bm.slaveEndpoint.ask[Boolean](RemoveBlock(blockId)).recover {
+ handleFailure("block", blockId.toString, bm.blockManagerId, false)
Review comment:
same comment as before.
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -177,6 +180,95 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
blockManager.stop()
}
+ private def setupBlockManagerMasterWithBlocks(withLost: Boolean): Unit = {
+ // set up a simple DriverEndpoint which simply adds executorIds and
+ // check whether a certain executorId has been added before.
Review comment:
nit:s/check/cheks/
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -235,7 +273,9 @@ class BlockManagerMasterEndpoint(
val removeMsg = RemoveShuffle(shuffleId)
Future.sequence(
blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Boolean](removeMsg)
+ bm.slaveEndpoint.ask[Boolean](removeMsg).recover {
+ handleFailure("shuffle", shuffleId.toString, bm.blockManagerId, false)
Review comment:
either add the comment as in the previous call, or pass by name here for clarity.
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,12 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no actual timeout property controls it.
+ * And timeout property should never be accessed since infinite means we never timeout.
+ * */
Review comment:
nit: `* */`, we use `*/` more commonly in spark.
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -93,6 +94,7 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
.set(MEMORY_STORAGE_FRACTION, 0.999)
.set(Kryo.KRYO_SERIALIZER_BUFFER_SIZE.key, "1m")
.set(STORAGE_UNROLL_MEMORY_THRESHOLD, 512L)
+ .set(Network.RPC_ASK_TIMEOUT, "5s")
Review comment:
Any particular reason why 5?
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656640107
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656645123
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650811678
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650866278
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650866278
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654571907
cc @cloud-fan Could you also take a look? Thanks.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654348285
**[Test build #125103 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125103/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650811678
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656519685
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652980402
**[Test build #124903 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124903/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650865545
retest this please.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650870142
**[Test build #124620 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124620/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654348675
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654653686
**[Test build #125186 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125186/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654631941
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/125169/
Test 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.
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] cloud-fan commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656680590
thanks, 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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452658224
##########
File path: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedClusterMessage.scala
##########
@@ -132,4 +132,6 @@ private[spark] object CoarseGrainedClusterMessages {
// Used internally by executors to shut themselves down.
case object Shutdown extends CoarseGrainedClusterMessage
+ // The message to check if scheduler thinks the executor is alive or not.
Review comment:
updated to `CoarseGrainedSchedulerBackend`
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652980654
thanks for the review. I've updated the PR. Please take another look:)
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656644069
**[Test build #125578 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125578/testReport)** for PR 28924 at commit [`319d925`](https://github.com/apache/spark/commit/319d9257b200bb087afe15aace8b66d96925ce93).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654573918
**[Test build #125169 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125169/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-653097200
Merged build finished. Test 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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452163195
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,14 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no timeout configuration property that
+ * controls it. Therefore, we use "infinite" without any specific reason as its timeout
+ * configuration property. And its timeout property should never be accessed since infinite
+ * means we never timeout.
+ */
+ val infiniteTimeout = new RpcTimeout(Long.MaxValue.nanos, "infinite")
Review comment:
shall we upper case the name as it's a constant?
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452658502
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,14 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no timeout configuration property that
+ * controls it. Therefore, we use "infinite" without any specific reason as its timeout
+ * configuration property. And its timeout property should never be accessed since infinite
+ * means we never timeout.
+ */
+ val infiniteTimeout = new RpcTimeout(Long.MaxValue.nanos, "infinite")
Review comment:
sure, updated to `INFINITE_TIMEOUT`
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654572316
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r448945570
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -168,6 +173,37 @@ class BlockManagerMasterEndpoint(
stop()
}
+ private def handleFailure[T](
+ blockType: String,
+ blockId: String,
+ bmId: BlockManagerId,
+ defaultValue: T): PartialFunction[Throwable, T] = {
Review comment:
Good idea!
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656534098
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452660165
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -177,6 +181,95 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
blockManager.stop()
}
+ private def setupBlockManagerMasterWithBlocks(withLost: Boolean): Unit = {
+ // set up a simple DriverEndpoint which simply adds executorIds and
+ // checks whether a certain executorId has been added before.
+ val driverEndpoint = rpcEnv.setupEndpoint(CoarseGrainedSchedulerBackend.ENDPOINT_NAME,
+ new RpcEndpoint {
+ private val executorSet = mutable.HashSet[String]()
+ override val rpcEnv: RpcEnv = this.rpcEnv
+ override def receiveAndReply(context: RpcCallContext): PartialFunction[Any, Unit] = {
+ case CoarseGrainedClusterMessages.RegisterExecutor(executorId, _, _, _, _, _, _, _) =>
+ executorSet += executorId
+ context.reply(true)
+ case CoarseGrainedClusterMessages.IsExecutorAlive(executorId) =>
+ context.reply(executorSet.contains(executorId))
+ }
+ }
+ )
+
+ def createAndRegisterBlockManager(timeout: Boolean): BlockManagerId = {
+ val id = if (timeout) "timeout" else "normal"
+ val bmRef = rpcEnv.setupEndpoint(s"bm-$id", new RpcEndpoint {
+ override val rpcEnv: RpcEnv = this.rpcEnv
+ private def reply[T](context: RpcCallContext, response: T): Unit = {
+ if (timeout) {
+ Thread.sleep(conf.getTimeAsMs(Network.RPC_ASK_TIMEOUT.key) + 1000)
+ }
+ context.reply(response)
+ }
+
+ override def receiveAndReply(context: RpcCallContext): PartialFunction[Any, Unit] = {
+ case RemoveRdd(_) => reply(context, 1)
+ case RemoveBroadcast(_, _) => reply(context, 1)
+ case RemoveShuffle(_) => reply(context, true)
+ }
+ })
+ val bmId = BlockManagerId(s"exec-$id", "localhost", 1234, None)
+ master.registerBlockManager(bmId, Array.empty, 2000, 0, bmRef)
+ }
+
+ // set up normal bm1
+ val bm1Id = createAndRegisterBlockManager(false)
+ // set up bm2, which intentionally takes more time than RPC_ASK_TIMEOUT to
+ // remove rdd/broadcast/shuffle in order to raise timeout error
+ val bm2Id = createAndRegisterBlockManager(true)
+
+ driverEndpoint.askSync[Boolean](CoarseGrainedClusterMessages.RegisterExecutor(
+ bm1Id.executorId, null, bm1Id.host, 1, Map.empty, Map.empty,
+ Map.empty, 0))
+
+ if (!withLost) {
Review comment:
We always set up three roles, driver, executor-1, executor-2 before these two tests. `withLost` here indicates whether to initialize the executor-2 as a lost executor in terms of driver's view.
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656721945
thanks all!!
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656519264
**[Test build #125568 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125568/testReport)** for PR 28924 at commit [`828aa2b`](https://github.com/apache/spark/commit/828aa2bbdbf097e8fde22c42d780673852537efb).
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648891994
Merged build finished. Test 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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654788834
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r448951365
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -235,7 +269,10 @@ class BlockManagerMasterEndpoint(
val removeMsg = RemoveShuffle(shuffleId)
Future.sequence(
blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Boolean](removeMsg)
+ bm.slaveEndpoint.ask[Boolean](removeMsg).recover {
+ // use false as default value means no shuffle data were removed
+ handleFailure("shuffle", shuffleId.toString, bm.blockManagerId, false)
Review comment:
I think we may miss it previously. The failure of removing blocks should be considered as non-fatal. For example, it's not worth failing the whole application if we fail to remove some blocks just after some heavy computation.
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654468770
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/125103/
Test 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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648891546
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650912895
**[Test build #124620 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124620/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` case class IsExecutorAlive(executorId: String) extends CoarseGrainedClusterMessage`
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654631932
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656534098
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654571825
retest this please
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656519264
**[Test build #125568 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125568/testReport)** for PR 28924 at commit [`828aa2b`](https://github.com/apache/spark/commit/828aa2bbdbf097e8fde22c42d780673852537efb).
----------------------------------------------------------------
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.
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] cloud-fan closed pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #28924:
URL: https://github.com/apache/spark/pull/28924
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-653095825
**[Test build #124903 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124903/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654468764
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r447558818
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -95,6 +97,13 @@ class BlockManagerMasterEndpoint(
private val externalShuffleServiceRddFetchEnabled: Boolean = externalBlockStoreClient.isDefined
private val externalShuffleServicePort: Int = StorageUtils.externalShuffleServicePort(conf)
+ private lazy val driverEndpoint = {
Review comment:
sounds good!
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-651899756
**[Test build #124660 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124660/testReport)** for PR 28924 at commit [`5a02ef2`](https://github.com/apache/spark/commit/5a02ef2f013c816165e97f3c9ec0eded2a2d9a03).
----------------------------------------------------------------
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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452077031
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala
##########
@@ -166,8 +167,8 @@ class BlockManagerMaster(
s" with removeFromMaster = $removeFromMaster - ${e.getMessage}", e)
)(ThreadUtils.sameThread)
if (blocking) {
- timeout.awaitResult(future)
- }
+ // the underlying Futures will timeout anyway, so it's safe to use infinite here
+ infinite.awaitResult(future) }
Review comment:
ditto
----------------------------------------------------------------
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.
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 change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r448951365
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -235,7 +269,10 @@ class BlockManagerMasterEndpoint(
val removeMsg = RemoveShuffle(shuffleId)
Future.sequence(
blockManagerInfo.values.map { bm =>
- bm.slaveEndpoint.ask[Boolean](removeMsg)
+ bm.slaveEndpoint.ask[Boolean](removeMsg).recover {
+ // use false as default value means no shuffle data were removed
+ handleFailure("shuffle", shuffleId.toString, bm.blockManagerId, false)
Review comment:
I think we miss it previously. The failure of removing blocks should be considered as non-fatal. For example, it's not worth failing the whole application if we fail to remove some blocks just after some heavy computation.
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654468764
Merged build finished. Test 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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650870142
**[Test build #124620 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/124620/testReport)** for PR 28924 at commit [`a09990f`](https://github.com/apache/spark/commit/a09990f4c1f9b5672ffe1856641d051e1eb77e86).
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652074346
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656533667
**[Test build #125578 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125578/testReport)** for PR 28924 at commit [`319d925`](https://github.com/apache/spark/commit/319d9257b200bb087afe15aace8b66d96925ce93).
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648891546
----------------------------------------------------------------
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.
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] holdenk commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
holdenk commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-651412453
Also for `user facing` change maybe "less failures" which is good and we should call out here so we can mention it in the release notes and encourage folks to upgrade.
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-648892005
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/124490/
Test 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.
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 a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r451717626
##########
File path: core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala
##########
@@ -168,6 +177,37 @@ class BlockManagerMasterEndpoint(
stop()
}
+ private def handleFailure[T](
Review comment:
javadoc :-)
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654787187
**[Test build #125186 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125186/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] tgravescs commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r447684033
##########
File path: core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedClusterMessage.scala
##########
@@ -132,4 +132,6 @@ private[spark] object CoarseGrainedClusterMessages {
// Used internally by executors to shut themselves down.
case object Shutdown extends CoarseGrainedClusterMessage
+ // The message to check whether the executor is alive or not.
Review comment:
might be nice just to clarify saying check if scheduler thinks executor is alive or not
##########
File path: core/src/main/scala/org/apache/spark/util/RpcUtils.scala
##########
@@ -54,6 +56,13 @@ private[spark] object RpcUtils {
RpcTimeout(conf, Seq(RPC_LOOKUP_TIMEOUT.key, NETWORK_TIMEOUT.key), "120s")
}
+ /**
+ * Infinite timeout is used internally, so there's no actual timeout property controls it.
Review comment:
maybe say "timeout configuration property that controls it".
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654572316
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-653097200
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-650949521
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654573918
**[Test build #125169 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125169/testReport)** for PR 28924 at commit [`a04a74e`](https://github.com/apache/spark/commit/a04a74e7909b53f7a3c53f86ea429b647d5a6b8a).
----------------------------------------------------------------
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.
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 #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656640107
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-652981089
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-656645123
----------------------------------------------------------------
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.
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 removed a comment on pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28924:
URL: https://github.com/apache/spark/pull/28924#issuecomment-654631932
Merged build finished. Test 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.
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] cloud-fan commented on a change in pull request #28924: [SPARK-32091][CORE] Ignore timeout error when remove blocks on the lost executor
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28924:
URL: https://github.com/apache/spark/pull/28924#discussion_r452661144
##########
File path: core/src/test/scala/org/apache/spark/storage/BlockManagerSuite.scala
##########
@@ -177,6 +181,95 @@ class BlockManagerSuite extends SparkFunSuite with Matchers with BeforeAndAfterE
blockManager.stop()
}
+ private def setupBlockManagerMasterWithBlocks(withLost: Boolean): Unit = {
Review comment:
can we add comment to explain what `withLost` means?
----------------------------------------------------------------
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.
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