You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/05/25 11:43:48 UTC
[GitHub] [spark] weixiuli opened a new pull request, #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
weixiuli opened a new pull request, #36665:
URL: https://github.com/apache/spark/pull/36665
### What changes were proposed in this pull request?
In the https://github.com/apache/spark/pull/34578 , it can ignore task finished events when some tasks are already finished state. But it may not be completely solved yet, because there is no need to set the task result size in accumulator updates received from the executor when task completion events should be ignored.
### Why are the changes needed?
Reduce unnecessary calculation when some tasks are already finished state.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Add unittests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
URL: https://github.com/apache/spark/pull/36665
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #36665:
URL: https://github.com/apache/spark/pull/36665#issuecomment-1254360691
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #36665:
URL: https://github.com/apache/spark/pull/36665#issuecomment-1137840908
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #36665:
URL: https://github.com/apache/spark/pull/36665#discussion_r886246382
##########
core/src/main/scala/org/apache/spark/scheduler/TaskResultGetter.scala:
##########
@@ -102,6 +102,10 @@ private[spark] class TaskResultGetter(sparkEnv: SparkEnv, scheduler: TaskSchedul
(deserializedResult, size)
}
+ // quickly return if the task has finished
+ if (scheduler.isFinishedTask(taskSetManager, tid)) {
+ return
Review Comment:
Agree with @Ngone51, this is something which is checked below (in `handleSuccessfulTask`) - the potential improvement, for an unlikely corner case, is not worth the additional complexity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Ngone51 commented on a diff in pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
Posted by GitBox <gi...@apache.org>.
Ngone51 commented on code in PR #36665:
URL: https://github.com/apache/spark/pull/36665#discussion_r885746852
##########
core/src/main/scala/org/apache/spark/scheduler/TaskResultGetter.scala:
##########
@@ -102,6 +102,10 @@ private[spark] class TaskResultGetter(sparkEnv: SparkEnv, scheduler: TaskSchedul
(deserializedResult, size)
}
+ // quickly return if the task has finished
+ if (scheduler.isFinishedTask(taskSetManager, tid)) {
+ return
Review Comment:
This's only useful when you hit the race condition, which is only a corner case. And in SPARK-37300, I think our target is to fix the bug issue but not for improvement. I personally think this introduces more complexity compared to the benefit we could get.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] weixiuli commented on pull request #36665: [SPARK-39287][CORE] TaskSchedulerImpl should quickly ignore task finished event if its task was finished state.
Posted by GitBox <gi...@apache.org>.
weixiuli commented on PR #36665:
URL: https://github.com/apache/spark/pull/36665#issuecomment-1140696630
ping @sleep1661 @Ngone51
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org