[SPARK-12887] Do not expose var's in TaskMetrics #10815

andrewor14 · 2016-01-18T20:56:36Z

This is a step in implementing SPARK-10620, which migrates TaskMetrics to accumulators.

TaskMetrics has a bunch of var's, some are fully public, some are private[spark]. This is bad coding style that makes it easy to accidentally overwrite previously set metrics. This has happened a few times in the past and caused bugs that were difficult to debug.

Instead, we should have get-or-create semantics, which are more readily understandable. This makes sense in the case of TaskMetrics because these are just aggregated metrics that we want to collect throughout the task, so it doesn't matter who's incrementing them.

Parent PR: #10717

Note: there's one remaining place, which is JsonProtocol.

JsonProtocol remains the only place where we still call set on each of the *Metrics classes.

This commit collapsed 10 methods into 2. The 8 that were inlined were only used in 1 place each, and the body of each was quite small. The additional level of abstraction did not add much value and made the code verbose.

JoshRosen · 2016-01-18T20:57:24Z

This is going to conflict with #10776, so I'd like to merge my patch first.

…etrics Conflicts: core/src/main/scala/org/apache/spark/CacheManager.scala core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala core/src/test/scala/org/apache/spark/CacheManagerSuite.scala

andrewor14 · 2016-01-18T22:16:55Z

retest this please

…etrics

JoshRosen · 2016-01-18T23:11:13Z

core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala

+      // that one. Otherwise, if the read method is different from the one previously seen by
+      // this task, we return a new dummy one to avoid clobbering the values of the old metrics.
+      // In the future we should try to store input metrics from all different read methods at
+      // the same time (SPARK-5225).


Hmm, that's unfortunate. Since it's a pre-existing problem, it's fine to not fix it here.

Before, I guess we'd unconditionally overwrite so we might handle it wrong even for the same read method case? This looks good to me, just curious RE: whether this fixed another bug.

SparkQA · 2016-01-18T23:24:28Z

Test build #49615 has finished for PR 10815 at commit 34c7ce5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2016-01-18T23:32:40Z

core/src/main/scala/org/apache/spark/rdd/PairRDDFunctions.scala

    }

    self.context.runJob(self, writeToFile)
    writer.commitJob()
  }

-  private def initHadoopOutputMetrics(context: TaskContext): (OutputMetrics, Option[() => Long]) = {
+  // TODO: these don't seem like the right abstractions.
+  // We should abstract the duplicate code in a less awkward way.


Agreed; this is kinda messy.

JoshRosen · 2016-01-18T23:56:21Z

I'm all finished with my review pass. I have two main pieces of feedback:

I think there's some opportunity to clean up the use of multiple Options in PairRDDFunctions, which could help to make the code easier to understand.
I have a question RE source/binary compatibility concerns for the changes in `TaskMetrics.

Aside from those things, this looks good.

SparkQA · 2016-01-19T00:26:47Z

Test build #49622 has finished for PR 10815 at commit e99b9af.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-01-19T00:35:26Z

Test build #49627 has finished for PR 10815 at commit e99b9af.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-01-19T02:41:50Z

Test build #49642 has finished for PR 10815 at commit c04b5df.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-01-19T03:04:44Z

Test build #49646 has finished for PR 10815 at commit 269031f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2016-01-19T03:07:53Z

Okay, so it turns out that I was wrong about the var -> def issue:

scala> class Foo {
     |    var field = 1
     | }
defined class Foo

becomes

scala> :javap -s - Foo
Compiled from "<console>"
public class  {
  public static final  MODULE$;
    descriptor: L;
  public static {};
    descriptor: ()V

  public scala.tools.nsc.interpreter.IMain $intp();
    descriptor: ()Lscala/tools/nsc/interpreter/IMain;

  public ();
    descriptor: ()V
}
Compiled from "<console>"
public class Foo {
  public int field();
    descriptor: ()I

  public void field_$eq(int);
    descriptor: (I)V

  public Foo();
    descriptor: ()V
}

Do you mind rolling back the changes due to that? Sorry about that.

SparkQA · 2016-01-19T03:10:47Z

Test build #49632 has finished for PR 10815 at commit d2e4e23.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-01-19T03:10:59Z

retest this please

JoshRosen · 2016-01-19T03:26:34Z

Uh oh, this merge-conflicted. I'm going to try to fix the conflicts and will submit a PR against your PR. You can on-the-go merge-button it, then I'll retest to get this in.

SparkQA · 2016-01-19T03:37:25Z

Test build #49657 has finished for PR 10815 at commit 12bd943.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

…reate-metrics

JoshRosen · 2016-01-19T03:54:09Z

Fixed the conflicts at andrewor14#3

Fix merge conflicts in get-or-create-metrics PR

andrewor14 · 2016-01-19T06:16:21Z

retest this please

JoshRosen · 2016-01-19T09:36:11Z

LGTM, so feel free to merge.

SparkQA · 2016-01-19T10:39:42Z

Test build #49665 has finished for PR 10815 at commit bfb3c05.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2016-01-19T16:39:00Z

Jenkins retest this please

SparkQA · 2016-01-19T18:57:29Z

Test build #49687 has finished for PR 10815 at commit bfb3c05.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-01-19T18:57:48Z

Merged into master.

Andrew Or added 5 commits January 18, 2016 11:54

Add register* methods (get or create)

62c96e1

Clean up places where we set OutputMetrics

b9d7fbf

Note: there's one remaining place, which is JsonProtocol.

Replace set with register

0785984

JsonProtocol remains the only place where we still call set on each of the *Metrics classes.

Clean up JsonProtocol

ad094f0

This commit collapsed 10 methods into 2. The 8 that were inlined were only used in 1 place each, and the body of each was quite small. The additional level of abstraction did not add much value and made the code verbose.

Hide updatedBlocks

34c7ce5

andrewor14 mentioned this pull request Jan 18, 2016

[SPARK-10620][WIP] Migrate TaskMetrics to accumulators #10717

Closed

Merge branch 'master' of github.com:apache/spark into get-or-create-m…

e99b9af

…etrics Conflicts: core/src/main/scala/org/apache/spark/CacheManager.scala core/src/main/scala/org/apache/spark/memory/StorageMemoryPool.scala core/src/test/scala/org/apache/spark/CacheManagerSuite.scala

andrewor14 force-pushed the get-or-create-metrics branch from 3948a5b to e99b9af Compare January 18, 2016 21:57

Andrew Or added 2 commits January 18, 2016 14:27

Merge branch 'master' of github.com:apache/spark into get-or-create-m…

202d48e

…etrics

One more

d2e4e23

JoshRosen reviewed Jan 18, 2016
View reviewed changes

andrewor14 force-pushed the get-or-create-metrics branch from 2e84e30 to 3e38670 Compare January 19, 2016 00:15

Review comments

c04b5df

andrewor14 force-pushed the get-or-create-metrics branch from 3e38670 to c04b5df Compare January 19, 2016 00:19

Remove unused method

269031f

Add back deprecated things

12bd943

Merge remote-tracking branch 'apache/master' into andrewor14-get-or-c…

2f1f6db

…reate-metrics

Merge pull request #3 from JoshRosen/andrewor14-get-or-create-metrics

bfb3c05

Fix merge conflicts in get-or-create-metrics PR

asfgit closed this in b122c86 Jan 19, 2016

andrewor14 deleted the get-or-create-metrics branch January 19, 2016 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-12887] Do not expose var's in TaskMetrics #10815

[SPARK-12887] Do not expose var's in TaskMetrics #10815

andrewor14 commented Jan 18, 2016

JoshRosen commented Jan 18, 2016

andrewor14 commented Jan 18, 2016

JoshRosen Jan 18, 2016

JoshRosen Jan 18, 2016

SparkQA commented Jan 18, 2016

JoshRosen Jan 18, 2016

JoshRosen commented Jan 18, 2016

SparkQA commented Jan 19, 2016

SparkQA commented Jan 19, 2016

SparkQA commented Jan 19, 2016

SparkQA commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

andrewor14 commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

andrewor14 commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

andrewor14 commented Jan 19, 2016

[SPARK-12887] Do not expose var's in TaskMetrics #10815

[SPARK-12887] Do not expose var's in TaskMetrics #10815

Conversation

andrewor14 commented Jan 18, 2016

JoshRosen commented Jan 18, 2016

andrewor14 commented Jan 18, 2016

JoshRosen Jan 18, 2016

Choose a reason for hiding this comment

JoshRosen Jan 18, 2016

Choose a reason for hiding this comment

SparkQA commented Jan 18, 2016

JoshRosen Jan 18, 2016

Choose a reason for hiding this comment

JoshRosen commented Jan 18, 2016

SparkQA commented Jan 19, 2016

SparkQA commented Jan 19, 2016

SparkQA commented Jan 19, 2016

SparkQA commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

andrewor14 commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

andrewor14 commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

JoshRosen commented Jan 19, 2016

SparkQA commented Jan 19, 2016

andrewor14 commented Jan 19, 2016