Skip to content

[SPARK-3465] fix task metrics aggregation in local mode #2338

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[SPARK-3465] fix task metrics aggregation in local mode #2338

wants to merge 4 commits into from

Conversation

@davies
Copy link
Contributor

@davies davies commented Sep 9, 2014

Before overwrite t.taskMetrics, take a deepcopy of it.

@davies
Copy link
Contributor Author

davies commented Sep 9, 2014

cc @sryza, is there a better way to fix it?

@sryza
Copy link
Contributor

sryza commented Sep 10, 2014

Hi @davies , sorry for causing this bug and thanks for picking it up. To avoid making the deep copy unnecessarily when running in non-local mode, we could instead make it on the executor side, and only do so if isLocal = true. Any issues you can see with that?

@davies
Copy link
Contributor Author

davies commented Sep 10, 2014

@sryza I had changed it to do the copy in Executor, then it's hard to write a test now. Any idea?

@SparkQA
Copy link

SparkQA commented Sep 10, 2014

QA tests have started for PR 2338 at commit 754b5b8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 10, 2014

Tests timed out after a configured wait of 120m.

@SparkQA
Copy link

SparkQA commented Sep 10, 2014

QA tests have started for PR 2338 at commit 754b5b8.

  • This patch does not merge cleanly!

@SparkQA
Copy link

SparkQA commented Sep 10, 2014

QA tests have finished for PR 2338 at commit 754b5b8.

  • This patch passes unit tests.
  • This patch does not merge cleanly!

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2338 at commit 754b5b8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 2338 at commit 754b5b8.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

andrewor14 commented Sep 11, 2014

LGTM

andrewor14
andrewor14 reviewed Sep 11, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/executor/Executor.scala Outdated
Copy link
Contributor

@andrewor14 andrewor14 Sep 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate in the comment more specifically why we need to do this?

@sryza
Copy link
Contributor

sryza commented Sep 11, 2014

I don't have any great ideas for how to write a test for it, but this looks good to me as well.

@andrewor14 andrewor14 mentioned this pull request Sep 11, 2014
Closed
@davies
Copy link
Contributor Author

davies commented Sep 11, 2014

@andrewor14 @sryza done

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 2338 at commit 7c879e0.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 12, 2014

QA tests have finished for PR 2338 at commit 7c879e0.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

andrewor14 commented Sep 12, 2014

Thanks @davies. This is going into master and 1.1.

@asfgit asfgit closed this in 42904b8 Sep 12, 2014
asfgit pushed a commit that referenced this pull request Sep 12, 2014
@davies @andrewor14
[SPARK-3465] fix task metrics aggregation in local mode
e69deb8 
Before overwrite t.taskMetrics, take a deepcopy of it.

Author: Davies Liu <[email protected]>

Closes #2338 from davies/fix_metric and squashes the following commits:

a5cdb63 [Davies Liu] Merge branch 'master' into fix_metric
7c879e0 [Davies Liu] add more comments
754b5b8 [Davies Liu] copy taskMetrics only when isLocal is true
5ca26dc [Davies Liu] fix task metrics aggregation in local mode
@davies davies deleted the fix_metric branch September 15, 2014 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@davies @sryza @SparkQA @andrewor14