-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-14704][CORE] create accumulators in TaskMetrics #12472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Test build #56065 has finished for PR 12472 at commit
|
|
ready for review :) |
|
Test build #56193 has finished for PR 12472 at commit
|
| protected[spark] def unset(): Unit = taskContext.remove() | ||
|
|
||
| /** | ||
| * An empty task context that does not represent an actual task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while you are at this, can you document this is only used for testing?
|
We should be able to remove almost all the methods on InternalAccumulators.scala, shouldn't we? All that includes create, createAll, createShuffleReadAccums, ... |
|
Test build #56204 has finished for PR 12472 at commit
|
|
Test build #56207 has finished for PR 12472 at commit
|
|
looks pretty good now |
|
Test build #56211 has finished for PR 12472 at commit
|
|
Test build #56227 has finished for PR 12472 at commit
|
|
retest this please |
|
Test build #56300 has finished for PR 12472 at commit
|
|
Test build #56306 has finished for PR 12472 at commit
|
|
Test build #56310 has finished for PR 12472 at commit
|
|
Thanks - merging in master. |
What changes were proposed in this pull request?
Before this PR, we create accumulators at driver side(and register them) and send them to executor side, then we create
TaskMetricswith these accumulators at executor side.After this PR, we will create
TaskMetricsat driver side and send it to executor side, so that we can create accumulators insideTaskMetricsdirectly, which is cleaner.How was this patch tested?
existing tests.