You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jinxing64 <gi...@git.apache.org> on 2017/01/30 05:04:39 UTC
[GitHub] spark pull request #16738: [SPARK-19398] remove one misleading log in TaskSe...
GitHub user jinxing64 opened a pull request:
https://github.com/apache/spark/pull/16738
[SPARK-19398] remove one misleading log in TaskSetManager.
## What changes were proposed in this pull request?
Log below is misleading:
```
if (successful(index)) {
logInfo(
s"Task ${info.id} in stage ${taskSet.id} (TID $tid) failed, " +
"but another instance of the task has already succeeded, " +
"so not re-queuing the task to be re-executed.")
}
```
If fetch failed, the task is marked as successful in TaskSetManager:: handleFailedTask. Then log above will be printed. The successful just means task will not be scheduled any longer, not a real success.
## How was this patch tested?
Existing unit tests can cover this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jinxing64/spark SPARK-19398
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16738.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #16738
----
commit 992bd3ce21d8c7f606307e4d99d236b7de2d82fe
Author: jinxing <ji...@meituan.com>
Date: 2017-01-30T05:02:18Z
[SPARK-19398] remove one misleading log in TaskSetManager.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16738
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16738
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16738
The state of this change is OK, but seems to trivial. This doesn't communicate something substantially different.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
@kayousterhout
Would you please give a look at this ? It's great if you could help review this : )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16738
**[Test build #3556 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3556/testReport)** for PR 16738 at commit [`a2cfd69`](https://github.com/apache/spark/commit/a2cfd6959ba670fdfe806880e216978d3c27b94d).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the issue:
https://github.com/apache/spark/pull/16738
LGTM! Thanks for fixing this! I'll merge this tomorrow once tests pass and pending any further comments from @srowen
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
@srowen
Thanks a lot. I'll refine : )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16738
**[Test build #3556 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3556/testReport)** for PR 16738 at commit [`a2cfd69`](https://github.com/apache/spark/commit/a2cfd6959ba670fdfe806880e216978d3c27b94d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
@srowen @jasonmoore2k
Thanks a lot for reviewing this PR~
>Should successful and tasksSuccessful renamed to be completed and tasksCompleted?
How do you think about above and remove the log.
```
if (successful(index)) {
logInfo(
s"Task ${info.id} in stage ${taskSet.id} (TID $tid) failed, " +
"but another instance of the task has already succeeded, " +
"so not re-queuing the task to be re-executed.")
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
@kayousterhout
Thanks a lot for helping this pr thus far. I think the proposal is quite clear. I've already refined. Please take another look.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16738: [SPARK-19398] Change one misleading log in TaskSe...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on a diff in the pull request:
https://github.com/apache/spark/pull/16738#discussion_r99523053
--- Diff: core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala ---
@@ -799,7 +799,7 @@ private[spark] class TaskSetManager(
if (successful(index)) {
logInfo(
s"Task ${info.id} in stage ${taskSet.id} (TID $tid) failed, " +
- "but another instance of the task has already succeeded, " +
+ "but it is already marked as successful, " +
--- End diff --
How about
"Task ${info.id} in stage ${taskSet.id} (TID $tid) failed, but the task will not be re-executed (either because the task failed with a fetch failure, so the previous stage needs to be re-run, or because a different copy of the task has already succeeded)"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16738
CC @jasonmoore2k for https://github.com/apache/spark/pull/12751/files#diff-bad3987c83bd22d46416d3dd9d208e76L719
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16738: [SPARK-19398] Change one misleading log in TaskSe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/16738
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
Should `successful` and `tasksSuccessful` renamed to be `completed` and `tasksCompleted`?which I think make more sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16738: [SPARK-19398] Change one misleading log in TaskSe...
Posted by jinxing64 <gi...@git.apache.org>.
GitHub user jinxing64 reopened a pull request:
https://github.com/apache/spark/pull/16738
[SPARK-19398] Change one misleading log in TaskSetManager.
## What changes were proposed in this pull request?
Log below is misleading:
```
if (successful(index)) {
logInfo(
s"Task ${info.id} in stage ${taskSet.id} (TID $tid) failed, " +
"but another instance of the task has already succeeded, " +
"so not re-queuing the task to be re-executed.")
}
```
If fetch failed, the task is marked as successful in `TaskSetManager:: handleFailedTask`. Then log above will be printed. The `successful` just means task will not be scheduled any longer, not a real success.
## How was this patch tested?
Existing unit tests can cover this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jinxing64/spark SPARK-19398
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16738.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #16738
----
commit 6ccd9f5cb6c03bad6a7a01104a95c277218ffa2d
Author: jinxing <ji...@meituan.com>
Date: 2017-02-05T03:46:35Z
[SPARK-19398] Change one misleading log in TaskSetManager.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the issue:
https://github.com/apache/spark/pull/16738
I'd prefer to just change the log message and not rename successful to completed -- which I think is too ambiguous with tasks that have "completed" in the sense that they've failed (if we're going to do that change, I think there should be a bigger change to change all of the uses of successful to be consistent and to add better commenting, which is beyond the scope of this log-change PR).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by kayousterhout <gi...@git.apache.org>.
Github user kayousterhout commented on the issue:
https://github.com/apache/spark/pull/16738
Merged this to master. Thanks for the fix @jinxing64 -- these fixes to improve readability / usability of the code are super useful!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] remove one misleading log in TaskSetManage...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/16738
Rather than remove the comment, maybe it's better to clarify it as you say, to refer to being "completed" rather than "successful". I don't know enough to evaluate whether you're right about that change, but it seems reasonable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
@kayousterhout
Thanks a lot again : )
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16738: [SPARK-19398] Change one misleading log in TaskSe...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 closed the pull request at:
https://github.com/apache/spark/pull/16738
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
I just changed the log message, but not sure if it clear enough.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16738: [SPARK-19398] Change one misleading log in TaskSetManage...
Posted by jinxing64 <gi...@git.apache.org>.
Github user jinxing64 commented on the issue:
https://github.com/apache/spark/pull/16738
Thanks a lot for the comments. Actually Im still not sure how to change this log or even just remove it. I just think the log is confusing. It is printed out every FetchFailed. Please give some suggestions if possible. If this is too trival to fix, should I close this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org