[SPARK-26285][CORE] accumulator metrics sources for LongAccumulator and Doub… #23242

abellina · 2018-12-06T04:37:18Z

…leAccumulator

What changes were proposed in this pull request?

This PR implements metric sources for LongAccumulator and DoubleAccumulator, such that a user can register these accumulators easily and have their values be reported by the driver's metric namespace.

How was this patch tested?

Unit tests, and manual tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

…leAccumulator

tgravescs · 2018-12-07T14:57:49Z

ok to test

SparkQA · 2018-12-07T15:05:11Z

Test build #99831 has finished for PR 23242 at commit 45cfada.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AccumulatorSource extends Source

redsanket · 2018-12-08T05:31:42Z

examples/src/main/scala/org/apache/spark/examples/AccumulatorMetricsTest.scala

+/**
+ * Usage: AccumulatorMetricsTest [partitions] [numElem] [blockSize]
+ */
+object AccumulatorMetricsTest {


Example named as Test is a bit confusing i think... thoughts?

@redsanket yes at first I thought that, but there are other many other "examples" here with the suffix Test.

SparkQA · 2018-12-08T23:13:32Z

Test build #99877 has finished for PR 23242 at commit de531c8.

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

tgravescs · 2018-12-18T19:50:09Z

core/src/main/scala/org/apache/spark/metrics/source/AccumulatorSource.scala

+  override def metricRegistry: MetricRegistry = registry
+}
+
+class LongAccumulatorSource extends AccumulatorSource


it might be good to mark all of these are Experimental for now, that gives us an out if we need to change something with api until we get some usage of it.

Also add small description for scala docs for Long and Double Accumulator Source

tgravescs · 2018-12-18T20:06:54Z

core/src/main/scala/org/apache/spark/metrics/source/AccumulatorSource.scala

+ * the CollectionAccumulator, as that is a List of values (hard to report,
+ * to a metrics system)
+ */
+class AccumulatorSource extends Source {


I think we should make this private[spark] for now unless you have a use case that would need to public extend this

tgravescs · 2018-12-18T20:11:50Z

examples/src/main/scala/org/apache/spark/examples/AccumulatorMetricsTest.scala

+
+/**
+ * Usage: AccumulatorMetricsTest [partitions] [numElem] [blockSize]
+ */


add a description on what this does

tgravescs · 2018-12-18T20:12:38Z

examples/src/main/scala/org/apache/spark/examples/AccumulatorMetricsTest.scala

+
+    val spark = SparkSession
+      .builder()
+      .config("spark.broadcast.blockSize", blockSize)


we don't need this, assume copy from the BroadcastTest one

tgravescs · 2018-12-18T20:17:20Z

examples/src/main/scala/org/apache/spark/examples/AccumulatorMetricsTest.scala

+      val observedSizes = sc.parallelize(1 to 10, slices).map(_ => {
+        acc.add(1)
+        acc2.add(1.1)
+        barr1.value.length


I think we could simplify the test by remove the broadcast stuff as that isn't really what we are testing, just leave the accumulator updates and do simple map. Print the accumulators at the end. Also perhaps we should put more information the register part and how someone running this might see those metrics output

…al flags and comments

…spark into SPARK-26285_accumulator_source

abellina · 2018-12-18T23:24:06Z

@tgravescs I made the suggested changes, and made the example more relevant.

SparkQA · 2018-12-18T23:26:19Z

Test build #100292 has finished for PR 23242 at commit cf015cd.

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

SparkQA · 2018-12-19T03:46:37Z

Test build #100293 has finished for PR 23242 at commit 7f60730.

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

tgravescs · 2018-12-19T19:48:19Z

examples/src/main/scala/org/apache/spark/examples/AccumulatorMetricsTest.scala

+ *
+ * The only argument, numElem, sets the number elements in the collection to parallize.
+ *
+ * The result is output to stdout in the driver with the values of the accumulators.


perhaps we should expand this more saying using ConsoleSink so the metrics going to stdout show up as metrics like AccumulatorSource.my-double-metric

tgravescs · 2018-12-19T19:51:24Z

core/src/main/scala/org/apache/spark/metrics/source/AccumulatorSource.scala

+
+/**
+ * :: Experimental ::
+ * Metrics source specifically for LongAccumulators.


perhaps we should expand this and say user can register accumulators that show up in the spark driver metrics

SparkQA · 2018-12-22T01:53:09Z

Test build #100373 has finished for PR 23242 at commit 682cfd7.

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

tgravescs

+1

tgravescs · 2018-12-22T15:03:44Z

merged to master, thanks @abellina

…nd Doub… …leAccumulator ## What changes were proposed in this pull request? This PR implements metric sources for LongAccumulator and DoubleAccumulator, such that a user can register these accumulators easily and have their values be reported by the driver's metric namespace. ## How was this patch tested? Unit tests, and manual tests. Please review http://spark.apache.org/contributing.html before opening a pull request. Closes apache#23242 from abellina/SPARK-26285_accumulator_source. Lead-authored-by: Alessandro Bellina <abellina@yahoo-inc.com> Co-authored-by: Alessandro Bellina <abellina@oath.com> Co-authored-by: Alessandro Bellina <abellina@gmail.com> Signed-off-by: Thomas Graves <tgraves@apache.org>

SPARK-26285: accumulator metrics sources for LongAccumulator and Doub…

45cfada

…leAccumulator

abellina changed the title ~~SPARK-26285: accumulator metrics sources for LongAccumulator and Doub…~~ [SPARK-26285][CORE] accumulator metrics sources for LongAccumulator and Doub… Dec 6, 2018

redsanket reviewed Dec 8, 2018

View reviewed changes

SPARK-26285: scalastyle fixes

de531c8

tgravescs requested changes Dec 18, 2018

View reviewed changes

Alessandro Bellina added 3 commits December 18, 2018 23:21

SPARK-26285: make AccumulatorSource private[spark] and add Experiment…

fa45015

…al flags and comments

Merge branch 'SPARK-26285_accumulator_source' of github.com:abellina/…

1fd7ebe

…spark into SPARK-26285_accumulator_source

SPARK-26285: make accumulator example more relevant, and add description

cf015cd

SPARK-26285: scalastyle fixes

7f60730

tgravescs reviewed Dec 19, 2018

View reviewed changes

SPARK-26285: code review comment fixes

682cfd7

tgravescs approved these changes Dec 22, 2018

View reviewed changes

asfgit closed this in 0a02d5c Dec 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26285][CORE] accumulator metrics sources for LongAccumulator and Doub… #23242

[SPARK-26285][CORE] accumulator metrics sources for LongAccumulator and Doub… #23242

abellina commented Dec 6, 2018

tgravescs commented Dec 7, 2018

SparkQA commented Dec 7, 2018

redsanket Dec 8, 2018

abellina Dec 8, 2018

SparkQA commented Dec 8, 2018

tgravescs Dec 18, 2018

tgravescs Dec 18, 2018

tgravescs Dec 18, 2018

tgravescs Dec 18, 2018

tgravescs Dec 18, 2018

tgravescs Dec 18, 2018

abellina commented Dec 18, 2018

SparkQA commented Dec 18, 2018

SparkQA commented Dec 19, 2018

tgravescs Dec 19, 2018

tgravescs Dec 19, 2018

SparkQA commented Dec 22, 2018

tgravescs left a comment

tgravescs commented Dec 22, 2018

[SPARK-26285][CORE] accumulator metrics sources for LongAccumulator and Doub… #23242

[SPARK-26285][CORE] accumulator metrics sources for LongAccumulator and Doub… #23242

Conversation

abellina commented Dec 6, 2018

What changes were proposed in this pull request?

How was this patch tested?

tgravescs commented Dec 7, 2018

SparkQA commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellina commented Dec 18, 2018

SparkQA commented Dec 18, 2018

SparkQA commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 22, 2018

tgravescs left a comment

Choose a reason for hiding this comment

tgravescs commented Dec 22, 2018