[SPARK-6627] Some clean-up in shuffle code. #5286

pwendell · 2015-03-31T06:06:15Z

Before diving into review #4450 I did a look through the existing shuffle
code to learn how it works. Unfortunately, there are some very
confusing things in this code. This patch makes a few small changes
to simplify things. It is not easily to concisely describe the changes
because of how convoluted the issues were, but they are fairly small
logically:

There is a trait named ShuffleBlockManager that only deals with
one logical function which is retrieving shuffle block data given shuffle
block coordinates. This trait has two implementors FileShuffleBlockManager
and IndexShuffleBlockManager. Confusingly the vast majority of those
implementations have nothing to do with this particular functionality.
So I've renamed the trait to ShuffleBlockResolver and documented it.
The aforementioned trait had two almost identical methods, for no good
reason. I removed one method (getBytes) and modified callers to use the
other one. I think the behavior is preserved in all cases.
The sort shuffle code uses an identifier "0" in the reduce slot of a
BlockID as a placeholder. I made it into a constant since it needs to
be consistent across multiple places.

I think for (3) there is actually a better solution that would avoid the
need to do this type of workaround/hack in the first place, but it's more
complex so I'm punting it for now.

Before diving into review apache#4450 I did a look through the existing shuffle code. Unfortunately, there are some very confusing things in this code. This patch makes a few small changes to simplify things. It is not easily to concisely describe the changes because of how convoluted the issues were: 1. There was a trait named ShuffleBlockManager that only deals with one logical function which is retrieving shuffle block data given shuffle block coordinates. This trait has two implementors FileShuffleBlockManager and IndexShuffleBlockManager. Confusingly the vast majority of those implementations have nothing to do with this particular functionality. So I've renamed the trait to ShuffleBlockResolver and documented it. 2. The aformentioned trait had two almost identical methods, for no good reason. I removed one method (getBytes) and modified callers to use the other one. I think the behavior is preserved in all cases. 3. The sort shuffle code uses an identifier "0" in the reduce slot of a BlockID as a placeholder. I made it into a constant since it needs to be consistent across multiple places. I think for (3) there is actually a better solution that would avoid the need to do this type of workaround/hack in the first place, but it's more complex so I'm punting it for now.

pwendell · 2015-03-31T06:07:41Z

/cc @aarondav and @rxin, with whom I discussed some of the existing design. And @sryza who recently looked a lot at this code.

SparkQA · 2015-03-31T06:08:29Z

Test build #29456 has started for PR 5286 at commit a406079.

pwendell · 2015-03-31T06:08:32Z

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

This behavior is kind of lame (just wrapping this in a Some), but it preserves what was there before.

can you change Some -> Option, just in case nioByteBuffer returns null?

SparkQA · 2015-03-31T06:09:29Z

Test build #29456 has finished for PR 5286 at commit a406079.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-03-31T06:09:31Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29456/
Test FAILed.

rxin · 2015-03-31T06:12:54Z

Why is this a HOTFIX?

pwendell · 2015-03-31T06:17:20Z

We have to put that whenever we don't create a JIRA or else the scripts we use get messed up. I can just make a JIRA for the overall clean-up.

rxin · 2015-03-31T06:18:09Z

At a high level LGTM

SparkQA · 2015-03-31T06:23:13Z

Test build #29458 has started for PR 5286 at commit d1c0494.

SparkQA · 2015-03-31T07:08:27Z

Test build #29458 has finished for PR 5286 at commit d1c0494.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-03-31T07:08:31Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29458/
Test FAILed.

aarondav · 2015-03-31T07:18:06Z

core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala

Please continue

aarondav · 2015-03-31T07:21:25Z

LGTM too

SparkQA · 2015-03-31T16:53:20Z

Test build #29491 has started for PR 5286 at commit f36edd5.

SparkQA · 2015-03-31T17:38:32Z

Test build #29491 has finished for PR 5286 at commit f36edd5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-03-31T17:38:36Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29491/
Test FAILed.

pwendell · 2015-03-31T17:49:59Z

Jenkins, retest this pleas.e

pwendell · 2015-04-01T23:31:51Z

Jenkins, retest this please.

SparkQA · 2015-04-01T23:33:30Z

Test build #29576 has started for PR 5286 at commit f36edd5.

SparkQA · 2015-04-02T00:31:49Z

Test build #29576 has finished for PR 5286 at commit f36edd5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.
This patch removes the following dependencies:
- RoaringBitmap-0.4.5.jar
- activation-1.1.jar
- akka-actor_2.10-2.3.4-spark.jar
- akka-remote_2.10-2.3.4-spark.jar
- akka-slf4j_2.10-2.3.4-spark.jar
- aopalliance-1.0.jar
- arpack_combined_all-0.1.jar
- avro-1.7.7.jar
- breeze-macros_2.10-0.11.2.jar
- breeze_2.10-0.11.2.jar
- chill-java-0.5.0.jar
- chill_2.10-0.5.0.jar
- commons-beanutils-1.7.0.jar
- commons-beanutils-core-1.8.0.jar
- commons-cli-1.2.jar
- commons-codec-1.10.jar
- commons-collections-3.2.1.jar
- commons-compress-1.4.1.jar
- commons-configuration-1.6.jar
- commons-digester-1.8.jar
- commons-httpclient-3.1.jar
- commons-io-2.1.jar
- commons-lang-2.5.jar
- commons-lang3-3.3.2.jar
- commons-math-2.1.jar
- commons-math3-3.1.1.jar
- commons-net-2.2.jar
- compress-lzf-1.0.0.jar
- config-1.2.1.jar
- core-1.1.2.jar
- curator-client-2.4.0.jar
- curator-framework-2.4.0.jar
- curator-recipes-2.4.0.jar
- gmbal-api-only-3.0.0-b023.jar
- grizzly-framework-2.1.2.jar
- grizzly-http-2.1.2.jar
- grizzly-http-server-2.1.2.jar
- grizzly-http-servlet-2.1.2.jar
- grizzly-rcm-2.1.2.jar
- groovy-all-2.3.7.jar
- guava-14.0.1.jar
- guice-3.0.jar
- hadoop-annotations-2.2.0.jar
- hadoop-auth-2.2.0.jar
- hadoop-client-2.2.0.jar
- hadoop-common-2.2.0.jar
- hadoop-hdfs-2.2.0.jar
- hadoop-mapreduce-client-app-2.2.0.jar
- hadoop-mapreduce-client-common-2.2.0.jar
- hadoop-mapreduce-client-core-2.2.0.jar
- hadoop-mapreduce-client-jobclient-2.2.0.jar
- hadoop-mapreduce-client-shuffle-2.2.0.jar
- hadoop-yarn-api-2.2.0.jar
- hadoop-yarn-client-2.2.0.jar
- hadoop-yarn-common-2.2.0.jar
- hadoop-yarn-server-common-2.2.0.jar
- ivy-2.4.0.jar
- jackson-annotations-2.4.0.jar
- jackson-core-2.4.4.jar
- jackson-core-asl-1.8.8.jar
- jackson-databind-2.4.4.jar
- jackson-jaxrs-1.8.8.jar
- jackson-mapper-asl-1.8.8.jar
- jackson-module-scala_2.10-2.4.4.jar
- jackson-xc-1.8.8.jar
- jansi-1.4.jar
- javax.inject-1.jar
- javax.servlet-3.0.0.v201112011016.jar
- javax.servlet-3.1.jar
- javax.servlet-api-3.0.1.jar
- jaxb-api-2.2.2.jar
- jaxb-impl-2.2.3-1.jar
- jcl-over-slf4j-1.7.10.jar
- jersey-client-1.9.jar
- jersey-core-1.9.jar
- jersey-grizzly2-1.9.jar
- jersey-guice-1.9.jar
- jersey-json-1.9.jar
- jersey-server-1.9.jar
- jersey-test-framework-core-1.9.jar
- jersey-test-framework-grizzly2-1.9.jar
- jets3t-0.7.1.jar
- jettison-1.1.jar
- jetty-util-6.1.26.jar
- jline-0.9.94.jar
- jline-2.10.4.jar
- jodd-core-3.6.3.jar
- json4s-ast_2.10-3.2.10.jar
- json4s-core_2.10-3.2.10.jar
- json4s-jackson_2.10-3.2.10.jar
- jsr305-1.3.9.jar
- jtransforms-2.4.0.jar
- jul-to-slf4j-1.7.10.jar
- kryo-2.21.jar
- log4j-1.2.17.jar
- lz4-1.2.0.jar
- management-api-3.0.0-b012.jar
- mesos-0.21.0-shaded-protobuf.jar
- metrics-core-3.1.0.jar
- metrics-graphite-3.1.0.jar
- metrics-json-3.1.0.jar
- metrics-jvm-3.1.0.jar
- minlog-1.2.jar
- netty-3.8.0.Final.jar
- netty-all-4.0.23.Final.jar
- objenesis-1.2.jar
- opencsv-2.3.jar
- oro-2.0.8.jar
- paranamer-2.6.jar
- parquet-column-1.6.0rc3.jar
- parquet-common-1.6.0rc3.jar
- parquet-encoding-1.6.0rc3.jar
- parquet-format-2.2.0-rc1.jar
- parquet-generator-1.6.0rc3.jar
- parquet-hadoop-1.6.0rc3.jar
- parquet-jackson-1.6.0rc3.jar
- protobuf-java-2.4.1.jar
- protobuf-java-2.5.0-spark.jar
- py4j-0.8.2.1.jar
- pyrolite-2.0.1.jar
- quasiquotes_2.10-2.0.1.jar
- reflectasm-1.07-shaded.jar
- scala-compiler-2.10.4.jar
- scala-library-2.10.4.jar
- scala-reflect-2.10.4.jar
- scalap-2.10.4.jar
- scalatest_2.10-2.2.1.jar
- slf4j-api-1.7.10.jar
- slf4j-log4j12-1.7.10.jar
- snappy-java-1.1.1.6.jar
- spark-bagel_2.10-1.4.0-SNAPSHOT.jar
- spark-catalyst_2.10-1.4.0-SNAPSHOT.jar
- spark-core_2.10-1.4.0-SNAPSHOT.jar
- spark-graphx_2.10-1.4.0-SNAPSHOT.jar
- spark-launcher_2.10-1.4.0-SNAPSHOT.jar
- spark-mllib_2.10-1.4.0-SNAPSHOT.jar
- spark-network-common_2.10-1.4.0-SNAPSHOT.jar
- spark-network-shuffle_2.10-1.4.0-SNAPSHOT.jar
- spark-repl_2.10-1.4.0-SNAPSHOT.jar
- spark-sql_2.10-1.4.0-SNAPSHOT.jar
- spark-streaming_2.10-1.4.0-SNAPSHOT.jar
- spire-macros_2.10-0.7.4.jar
- spire_2.10-0.7.4.jar
- stax-api-1.0.1.jar
- stream-2.7.0.jar
- tachyon-0.5.0.jar
- tachyon-client-0.5.0.jar
- uncommons-maths-1.2.2a.jar
- unused-1.0.0.jar
- xmlenc-0.52.jar
- xz-1.0.jar
- zookeeper-3.4.5.jar

AmplabJenkins · 2015-04-02T00:31:53Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29576/
Test FAILed.

pwendell · 2015-04-02T04:33:28Z

Jenkins, retest this please.

pwendell · 2015-04-02T04:33:59Z

/cc @brennonyork - this is giving me strange dependency messages even though the patch doesn't touch any pom files.

SparkQA · 2015-04-02T04:38:22Z

Test build #29585 has started for PR 5286 at commit c71fbc7.

SparkQA · 2015-04-02T05:59:55Z

Test build #29585 has finished for PR 5286 at commit c71fbc7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-02T05:59:59Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29585/
Test PASSed.

pwendell reviewed Mar 31, 2015
View reviewed changes

pwendell changed the title ~~[HOTFIX] Some clean-up in shuffle code.~~ [SPARK-6627] Some clean-up in shuffle code. Mar 31, 2015

Style fix

d1c0494

aarondav reviewed Mar 31, 2015
View reviewed changes

core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockManager.scala Outdated

Copy link

Contributor

aarondav Mar 31, 2015

Choose a reason for hiding this comment

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

Please continue

Code review feedback

f36edd5

Open interface back up for testing

c71fbc7

asfgit closed this in 6562787 Apr 2, 2015

[SPARK-6627] Some clean-up in shuffle code. #5286

[SPARK-6627] Some clean-up in shuffle code. #5286

Uh oh!

Conversation

pwendell commented Mar 31, 2015

Uh oh!

pwendell commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

pwendell Mar 31, 2015

Choose a reason for hiding this comment

Uh oh!

rxin Mar 31, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

AmplabJenkins commented Mar 31, 2015

Uh oh!

rxin commented Mar 31, 2015

Uh oh!

pwendell commented Mar 31, 2015

Uh oh!

rxin commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

AmplabJenkins commented Mar 31, 2015

Uh oh!

aarondav Mar 31, 2015

Choose a reason for hiding this comment

Uh oh!

aarondav commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

AmplabJenkins commented Mar 31, 2015

Uh oh!

pwendell commented Mar 31, 2015

Uh oh!

pwendell commented Apr 1, 2015

Uh oh!

SparkQA commented Apr 1, 2015

Uh oh!

SparkQA commented Apr 2, 2015

Uh oh!

AmplabJenkins commented Apr 2, 2015

Uh oh!

pwendell commented Apr 2, 2015

Uh oh!

pwendell commented Apr 2, 2015

Uh oh!

SparkQA commented Apr 2, 2015

Uh oh!

SparkQA commented Apr 2, 2015

Uh oh!

AmplabJenkins commented Apr 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants