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/10/20 11:25:59 UTC
[GitHub] [spark] hzyangkai opened a new pull request #30103: fix Executor will not be deleted when idle timeout while running on k…
hzyangkai opened a new pull request #30103:
URL: https://github.com/apache/spark/pull/30103
…8s whith dynamic resource allocation
<!--
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?
when the tracker of one executor have no active shuffle ,clear shuffleIds in it.
<!--
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.
-->
### Why are the changes needed?
when one job with shuffle operation was completed ,if hasActiveShuffle variable of the tracker of one executor change from true to false,then shuffleIds should be cleared.because shuffleIds is used to decide if the executor has shuffle files.if shuffleIds is not cleared ,in Tractor#updateTimeout function ,newDeadline will be Long.MaxValue,and then executor will not been deleted forever.
<!--
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.
-->
### Does this PR introduce _any_ user-facing change?
No
<!--
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'.
-->
### 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.
-->
----------------------------------------------------------------
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] hzyangkai commented on pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
hzyangkai commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-725311286
Jenkins, 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] tgravescs commented on a change in pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #30103:
URL: https://github.com/apache/spark/pull/30103#discussion_r523023927
##########
File path: core/src/main/scala/org/apache/spark/scheduler/dynalloc/ExecutorMonitor.scala
##########
@@ -586,6 +586,7 @@ private[spark] class ExecutorMonitor(
val hadActiveShuffle = hasActiveShuffle
hasActiveShuffle = ids.exists(shuffleIds.contains)
if (hadActiveShuffle && isIdle) {
+ shuffleIds.clear()
Review comment:
I don't follow this change. Here isIdle means we don't have any running tasks and hasActiveshuffle is false. The ids passed in are supposed to be all the active ones which is determined by shuffleToActiveJobs. It does not mean that shuffleIds is empty. shuffleIds stick around until we get the ShuffleCleanedEvent that comes from context cleaner when RDD isn't referenced anymore. If its referenced even though the job may be done it means someone could use that still so we don't remove it until after the timeout. Here is we are idle and had active shuffle we update the Timeout, which should be setting it to either cache or storage timeout and then when the ExecutorMonitor comes around to check it should see that its set and time it out regardless of if it has shuffle ids.
If there is a bug in there somewhere then we need to figure out where it is. I do not think is not the correct fix.
----------------------------------------------------------------
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] hzyangkai commented on pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
hzyangkai commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-726258088
> I haven't reviewed yet, but test failure is in executor monitor tests so likely related to this change, please take a look.
thanks for your reply, i have some idea for the test failure.
I think the parameter spark.dynamicAllocation.shuffleTracking.timeout is unreasonable.
According to the spark documentation：this option can be used to control when to time out executors even when they are
storing shuffle data.it indicates that the downstream task will fail if the execution time exceeds this configuration.
But I think this is more reasonable: if all shuffles contained in an executor are not used by an active job, then the executor can be deleted at this time.
i am Looking forward to your reply.
----------------------------------------------------------------
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-725448518
I haven't reviewed yet, but test failure is in executor monitor tests so likely related to this change, please take a 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] hzyangkai commented on a change in pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
hzyangkai commented on a change in pull request #30103:
URL: https://github.com/apache/spark/pull/30103#discussion_r524989412
##########
File path: core/src/main/scala/org/apache/spark/scheduler/dynalloc/ExecutorMonitor.scala
##########
@@ -586,6 +586,7 @@ private[spark] class ExecutorMonitor(
val hadActiveShuffle = hasActiveShuffle
hasActiveShuffle = ids.exists(shuffleIds.contains)
if (hadActiveShuffle && isIdle) {
+ shuffleIds.clear()
Review comment:
thank you for your explaining. i got it .it seems works well . i will close this issue.
----------------------------------------------------------------
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-712785149
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.
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
tgravescs commented on a change in pull request #30103:
URL: https://github.com/apache/spark/pull/30103#discussion_r523023927
##########
File path: core/src/main/scala/org/apache/spark/scheduler/dynalloc/ExecutorMonitor.scala
##########
@@ -586,6 +586,7 @@ private[spark] class ExecutorMonitor(
val hadActiveShuffle = hasActiveShuffle
hasActiveShuffle = ids.exists(shuffleIds.contains)
if (hadActiveShuffle && isIdle) {
+ shuffleIds.clear()
Review comment:
I don't follow this change. Here isIdle means we don't have any running tasks and hasActiveshuffle is false. The ids passed in are supposed to be all the active ones which is determined by shuffleToActiveJobs. It does not mean that shuffleIds is empty. shuffleIds stick around until we get the ShuffleCleanedEvent that comes from context cleaner when RDD isn't referenced anymore. If its referenced even though the job may be done it means someone could use that still so we don't remove it until after the timeout. Here if we are idle and had active shuffle we update the Timeout, which should be setting it to either cache or storage timeout and then when the ExecutorMonitor comes around to check it should see that its set and time it out regardless of if it has shuffle ids.
If there is a bug in there somewhere then we need to figure out where it is. I do not think is not the correct fix.
----------------------------------------------------------------
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 #30103: fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-712785149
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.
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] hzyangkai commented on pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
hzyangkai commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-713497753
test failed not related to this change, what should i do @HyukjinKwon
----------------------------------------------------------------
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-712785793
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.
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #30103:
URL: https://github.com/apache/spark/pull/30103
----------------------------------------------------------------
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-712785793
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.
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] HyukjinKwon commented on pull request #30103: fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-712784019
@hzyangkai, can you file a JIRA and link it to the PR title? See also http://spark.apache.org/contributing.html
----------------------------------------------------------------
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-726828759
there is not enough information in the jira or description for someone to reproduce, can you please either write a test (ideal if a fix is going to go in) or at least provide manual steps to reproduce.
----------------------------------------------------------------
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 #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
tgravescs commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-726815252
I don't want to change the definition of that config at this point. I don't see why your change here would be affected by that? Other then are you saying without this change the executors stay around and is able to shuffle the data for this test?
----------------------------------------------------------------
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] hzyangkai commented on pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
hzyangkai commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-712798928
> @hzyangkai, can you file a JIRA and link it to the PR title? See also http://spark.apache.org/contributing.html
thanks, done
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-786329829
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.
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] hzyangkai commented on pull request #30103: [SPARK-33192][CORE]fix Executor will not be deleted when idle timeout while running on k…
Posted by GitBox <gi...@apache.org>.
hzyangkai commented on pull request #30103:
URL: https://github.com/apache/spark/pull/30103#issuecomment-725317932
hi, @mridulm ,could you help me review this pr please? thanks very much.
----------------------------------------------------------------
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