SPARK-2566. Update ShuffleWriteMetrics incrementally #1481

sryza · 2014-07-18T08:28:32Z

I haven't tested this out on a cluster yet, but wanted to make sure the approach (passing ShuffleWriteMetrics down to DiskBlockObjectWriter) was ok

SparkQA · 2014-07-18T08:33:04Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16822/consoleFull

SparkQA · 2014-07-18T10:12:57Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16822/consoleFull

SparkQA · 2014-07-18T23:58:00Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16839/consoleFull

SparkQA · 2014-07-19T01:37:06Z

QA results for PR 1481:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16839/consoleFull

SparkQA · 2014-08-01T20:09:29Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17696/consoleFull

pwendell · 2014-08-01T20:43:16Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

@@ -567,10 +567,12 @@ private[spark] class BlockManager(
      blockId: BlockId,
      file: File,
      serializer: Serializer,
-      bufferSize: Int): BlockObjectWriter = {
+      bufferSize: Int,
+      writeMetrics: Option[ShuffleWriteMetrics] = None): BlockObjectWriter = {


There is a bit of a mismatch in this function signature, the doc says "This is currently used for writing shuffle files out.", but the shuffle write metrics are optional. I think the doc is just stale, because we now use this in other cases where we might not be writing out shuffle data, so can you just remove that line from the doc?

SparkQA · 2014-08-01T21:03:57Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17696/consoleFull

pwendell · 2014-08-01T21:15:20Z

I'm not totally finished with this but a couple high level comments:

I think this entirely subsumes some existing code that tracks timing only in order to bubble it up to the write metrics, so if that code could be removed it would be great (it will simplify things and maybe this patch can even net remove code).
The fields in ShuffleWriteMetrics should be made volatile in order to ensure that they are visible when heartbeats happen. I think this should be fine from a performance perspective because at run-time the writer threads are really active and the reader threads just come by infrequently to hearbeat.

sryza · 2014-08-02T23:37:56Z

I hadn't noticed this before, but DiskObjectWriter is used for tracking bytes spilled by ExternalSorter and ExternalAppendOnlyMap in addition to shuffle bytes written. So in some cases it needs to bill to TaskMetrics.diskBytesSpilled as well. Any thoughts on the best way to handle this? The options as I see it are:

Pass in TaskMetrics and some flag about which metric to update to DiskObjectWriter
Pass in a callback that DiskObjectWriter can notify when it writes bytes
Pass in a MutableLong

pwendell · 2014-08-03T00:10:28Z

@sryza Can't the ExeternalSorter and ExternalAppendOnlyMap just pass their own ShuffleWriteMetrics when they create a disk writer and then read back the bytes written? I think that would be nicer actually because you wouldn't have this Option thing everywhere, bascially when you create a disk writer you need to supply metrics.

We could also change the name of ShuffleWriteMetrics to just be WriteMetrics - or we could leave it for now and just put a TODO.

pwendell · 2014-08-03T19:40:38Z

I guess the current name isn't that bad because technically this external aggregation does occur during a shuffle, although it's not exactly the write of the shuffle data itself. In any case I think making it not an option is a good call... hopefully it will simplify things.

SparkQA · 2014-08-03T20:19:28Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17827/consoleFull

sryza · 2014-08-03T20:20:01Z

Updated patch keeps it as ShuffleWriteMetrics for now.

SparkQA · 2014-08-03T20:20:13Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17827/consoleFull

SparkQA · 2014-08-03T20:24:17Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17828/consoleFull

pwendell · 2014-08-03T23:54:33Z

core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala

@@ -89,6 +90,8 @@ class ExternalAppendOnlyMap[K, V, C](
  // How much of the shared memory pool this collection has claimed
  private var myMemoryThreshold = 0L

+  private val writeMetrics = new ShuffleWriteMetrics


It's a bit strange that the same metrics object is re-used here. Can you just create a new ShuffleWriteMetrics every time you create a writer? That is more the model I was thinking of.

you can continue to track _diskBytesSpilled as its own var

pwendell · 2014-08-04T00:45:32Z

I tool a pass. I like the new approach, with this approach you actually net remove code from Spark, which is great. Left comments inline. This needs some unit tests as well. It shouldn't be too hard because you can create a BlockObjectWriter pretty easily (it is fairly self-contained thing). One thing that might be helpful for unit tests is adding a private flush method to BlockObjectWriter - since otherwise writes will get buffered inside of the inner streams and you might not ever see them in the resulting file if you are writing small amounts of data.

kayousterhout · 2014-08-04T07:14:40Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

@@ -29,7 +29,7 @@ import akka.actor.{ActorSystem, Cancellable, Props}
 import sun.nio.ch.DirectBuffer

 import org.apache.spark._
-import org.apache.spark.executor.{DataReadMethod, InputMetrics}
+import org.apache.spark.executor.{ShuffleWriteMetrics, DataReadMethod, InputMetrics}


alphabetization (here and elsewhere as well)

sryza · 2014-08-04T07:24:36Z

Updated patch addresses @pwendell and @kayousterhout 's comments and adds tests.

SparkQA · 2014-08-04T07:29:23Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17848/consoleFull

SparkQA · 2014-08-06T00:59:25Z

QA tests have started for PR 1481. This patch DID NOT merge cleanly!
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17964/consoleFull

SparkQA · 2014-08-06T01:04:19Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17965/consoleFull

SparkQA · 2014-08-06T01:58:13Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17965/consoleFull

SparkQA · 2014-08-06T02:52:09Z

QA results for PR 1481:
- This patch FAILED unit tests.

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17964/consoleFull

SparkQA · 2014-08-06T07:09:29Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18012/consoleFull

SparkQA · 2014-08-06T07:59:43Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18012/consoleFull

SparkQA · 2014-08-06T16:14:19Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18027/consoleFull

SparkQA · 2014-08-06T16:56:53Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18027/consoleFull

SparkQA · 2014-08-06T17:59:21Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18030/consoleFull

SparkQA · 2014-08-06T18:44:51Z

QA results for PR 1481:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18030/consoleFull

pwendell · 2014-08-06T19:08:31Z

Jenkins, test this please.

SparkQA · 2014-08-06T19:14:30Z

QA tests have started for PR 1481. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18039/consoleFull

SparkQA · 2014-08-06T20:04:02Z

QA results for PR 1481:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18039/consoleFull

kayousterhout · 2014-08-06T20:04:30Z

woohoooo!

On Wed, Aug 6, 2014 at 1:04 PM, Apache Spark QA notifications@github.com
wrote:

QA results for PR 1481:

This patch PASSES unit tests.

This patch merges cleanly

This patch adds no public classes

For more information see test ouptut:

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18039/consoleFull

—
Reply to this email directly or view it on GitHub
#1481 (comment).

pwendell · 2014-08-06T20:10:02Z

lol

pwendell · 2014-08-06T20:10:16Z

Okay I'm merging this - thanks Sandy!

sryza · 2014-08-06T20:13:14Z

hahaha I am filled with jubilation

sryza · 2014-08-06T20:13:44Z

thanks Patrick

I haven't tested this out on a cluster yet, but wanted to make sure the approach (passing ShuffleWriteMetrics down to DiskBlockObjectWriter) was ok Author: Sandy Ryza <sandy@cloudera.com> Closes #1481 from sryza/sandy-spark-2566 and squashes the following commits: 8090d88 [Sandy Ryza] Fix ExternalSorter b2a62ed [Sandy Ryza] Fix more test failures 8be6218 [Sandy Ryza] Fix test failures and mark a couple variables private c5e68e5 [Sandy Ryza] SPARK-2566. Update ShuffleWriteMetrics incrementally (cherry picked from commit 4e98236) Signed-off-by: Patrick Wendell <pwendell@gmail.com>

I haven't tested this out on a cluster yet, but wanted to make sure the approach (passing ShuffleWriteMetrics down to DiskBlockObjectWriter) was ok Author: Sandy Ryza <sandy@cloudera.com> Closes apache#1481 from sryza/sandy-spark-2566 and squashes the following commits: 8090d88 [Sandy Ryza] Fix ExternalSorter b2a62ed [Sandy Ryza] Fix more test failures 8be6218 [Sandy Ryza] Fix test failures and mark a couple variables private c5e68e5 [Sandy Ryza] SPARK-2566. Update ShuffleWriteMetrics incrementally

Sets boson configuration variables to true if the `BOSON` env variable is set to true. To run the unit tests, set ``` export BOSON=true export BOSON_CONF_DIR=$SPARK_SRC_DIR/boson/conf ```

pwendell reviewed Aug 1, 2014
View reviewed changes

pwendell reviewed Aug 3, 2014
View reviewed changes

kayousterhout reviewed Aug 4, 2014
View reviewed changes

SPARK-2566. Update ShuffleWriteMetrics incrementally

c5e68e5

Fix test failures and mark a couple variables private

8be6218

Fix more test failures

b2a62ed

Fix ExternalSorter

8090d88

asfgit closed this in 4e98236 Aug 6, 2014

SPARK-2566. Update ShuffleWriteMetrics incrementally #1481

SPARK-2566. Update ShuffleWriteMetrics incrementally #1481

Uh oh!

Conversation

sryza commented Jul 18, 2014

Uh oh!

SparkQA commented Jul 18, 2014

Uh oh!

SparkQA commented Jul 18, 2014

Uh oh!

SparkQA commented Jul 18, 2014

Uh oh!

SparkQA commented Jul 19, 2014

Uh oh!

SparkQA commented Aug 1, 2014

Uh oh!

pwendell Aug 1, 2014

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 1, 2014

Uh oh!

pwendell commented Aug 1, 2014

Uh oh!

sryza commented Aug 2, 2014

Uh oh!

pwendell commented Aug 3, 2014

Uh oh!

pwendell commented Aug 3, 2014

Uh oh!

SparkQA commented Aug 3, 2014

Uh oh!

sryza commented Aug 3, 2014

Uh oh!

SparkQA commented Aug 3, 2014

Uh oh!

SparkQA commented Aug 3, 2014

Uh oh!

pwendell Aug 3, 2014

Choose a reason for hiding this comment

Uh oh!

pwendell Aug 3, 2014

Choose a reason for hiding this comment

Uh oh!

pwendell commented Aug 4, 2014

Uh oh!

kayousterhout Aug 4, 2014

Choose a reason for hiding this comment

Uh oh!

sryza commented Aug 4, 2014

Uh oh!

SparkQA commented Aug 4, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

pwendell commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

SparkQA commented Aug 6, 2014

Uh oh!

kayousterhout commented Aug 6, 2014

Uh oh!