[SPARK-2532] Consolidated shuffle fixes #1609

mridulm · 2014-07-27T15:31:18Z

Status of the PR

Cherry pick and merge changes from internal branch to spark master
Remove WIP comments and 2G branch references.
Tests for BlockObjectWriter
Tests for ExternalAppendOnlyMap
Tests for ShuffleBlockManager

SparkQA · 2014-07-27T15:33:43Z

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

SparkQA · 2014-07-27T15:33:54Z

QA results for PR 1609:
- 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/17242/consoleFull

SparkQA · 2014-07-27T15:43:36Z

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

SparkQA · 2014-07-27T15:43:42Z

QA results for PR 1609:
- 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/17243/consoleFull

SparkQA · 2014-07-27T16:08:37Z

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

SparkQA · 2014-07-27T16:34:46Z

QA results for PR 1609:
- 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/17244/consoleFull

mateiz · 2014-07-27T23:20:51Z

core/src/test/scala/org/apache/spark/storage/DiskBlockObjectWriterSuite.scala

+
+  // val confCopy = conf.clone
+  // // Ensure we always write data after object ser
+  // confCopy.set("spark.serializer.objectStreamReset", "1")


Make sure to remove commented out debug code like this

This is already removed - waiting for tests to pass locally :-)

mateiz · 2014-07-27T23:21:37Z

@adav @andrewor14 would be good if you two take a look at this when it's merging correctly.

SparkQA · 2014-07-27T23:48:48Z

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

… into consolidated_shuffle_fixes

andrewor14 · 2014-07-28T00:12:37Z

Did you mean @aarondav?

SparkQA · 2014-07-28T00:34:52Z

QA results for PR 1609:
- 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/17256/consoleFull

witgo · 2014-07-28T05:22:53Z

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

+      initialized = true
+      wasOpenedOnce = true;
+    } finally {
+      if (! initialized) {


One more spaces

aarondav · 2014-07-28T06:51:41Z

@mridulm Thanks for submitting this! I would like to dig a little deeper into understanding the specific issues you found, in order to understand the solutions you have provided (since the specific solutions seem interleaved with a lot of new asserts and code paths).

You mention that there was an issue if shuffle writes co-occur with shuffle fetches, which is true, but should not typically occur due to the barrier before the reduce stage of a shuffle. In what situations does this happen (outside of failure conditions)?

Did you observe a prior pattern of close/revert/close on the same block writer?

How did task failures induce inconsistent state on the map side? Was it due to the same close/revert/close pattern?

aarondav · 2014-07-31T02:04:57Z

I created #1678, which only includes the changes directly related to fixing the issues with shuffle file consolidation (essentially forking off a piece of this PR), intended as a simple candidate for review to make the 1.1 release. The smaller PR is not intended as a replacement for this more complete one, however -- it is merely an option to fix some of the more severe bugs in time for the next major release. If we can get this one in for 1.1 instead, then we should.

@mridulm

All changes from this PR are by @mridulm and are drawn from his work in apache#1609. This patch is intended to fix all major issues related to shuffle file consolidation that @mridulm found, while minimizing changes to the code, with the hope that it may be more easily merged into 1.1. This patch is **not** intended as a replacement for apache#1609, which provides many additional benefits, including fixes to ExternalAppendOnlyMap, improvements to DiskBlockObjectWriter's API, and several new unit tests. If it is feasible to merge apache#1609 for the 1.1 deadline, that is a preferable option.

mateiz · 2014-07-31T19:05:51Z

core/src/main/scala/org/apache/spark/util/Java7Util.scala

+ */
+object Java7Util {
+  def isSymlink(file: File) = java.nio.file.Files.isSymbolicLink(file.toPath)
+}


Will this still compile on Java 6? I don't see any changes to pom.xml or anything to exclude it. Or maybe did you mean to call this by reflection?

mateiz · 2014-07-31T19:08:07Z

So FYI I'm going to make a more detailed pass through this soon to see if we can get all of it into 1.1. It would be nice to get all these changes in so we can QA them along with the other QA we do for 1.1, but if that doesn't work out, we can split some of them into smaller patches as Aaron did.

mateiz · 2014-08-01T20:10:31Z

So I looked through this and I also think it would be good to split it into smaller patches for 1.1. As far as I can see there are several orthogonal improvements here:

Shuffle file consolidation fixes that Aaron copied in SPARK-2791: Fix committing, reverting and state tracking in shuffle file consolidation #1678
ExternalAppendOnlyMap fixes to deal with writes past end of stream; we also need these in ExternalSorter
Fixes to directory creation in DiskBlockManager (I'm still not sure when this would be a problem actually if all accesses to these directories are through getFile; needs some investigation)
Fixes to isSymlink (though as is this seems like it would only compile on Java 7)
Improvements to the API of DiskBlockObjectWriter

Of these, the first two are most critical. So I'd like to get those into 1.1, and then we can do API refactoring and the other fixes on the master branch. For the directory creation fix I'd still like to understand when that can be a problem (I'm probably just missing something), but it's also one we can add in 1.1 during the QA window.

I'm going to update the JIRA to create sub-tasks for these things so we can track where each one is fixed. Thanks again for putting this together Mridul, this is very helpful.

…ile consolidation All changes from this PR are by mridulm and are drawn from his work in #1609. This patch is intended to fix all major issues related to shuffle file consolidation that mridulm found, while minimizing changes to the code, with the hope that it may be more easily merged into 1.1. This patch is **not** intended as a replacement for #1609, which provides many additional benefits, including fixes to ExternalAppendOnlyMap, improvements to DiskBlockObjectWriter's API, and several new unit tests. If it is feasible to merge #1609 for the 1.1 deadline, that is a preferable option. Author: Aaron Davidson <[email protected]> Closes #1678 from aarondav/consol and squashes the following commits: 53b3f6d [Aaron Davidson] Correct behavior when writing unopened file 701d045 [Aaron Davidson] Rebase with sort-based shuffle 9160149 [Aaron Davidson] SPARK-2532: Minimal shuffle consolidation fixes

@mridulm

All these changes are from @mridulm's work in apache#1609, but extracted here to fix this specific issue. This particular set of changes is to make sure that we read exactly the right range of bytes from each spill file in EAOM: some serializers can write bytes after the last object (e.g. the TC_RESET flag in Java serialization) and that would confuse the previous code into reading it as part of the next batch. There are also improvements to cleanup to make sure files are closed.

mateiz · 2014-08-01T22:46:44Z

Opened #1722 to do the second fix (map batch writing) for 1.1, including applying the same fix to ExternalSorter.

@mridulm

…in ExternalMap / Sorter All these changes are from mridulm's work in #1609, but extracted here to fix this specific issue and make it easier to merge not 1.1. This particular set of changes is to make sure that we read exactly the right range of bytes from each spill file in EAOM: some serializers can write bytes after the last object (e.g. the TC_RESET flag in Java serialization) and that would confuse the previous code into reading it as part of the next batch. There are also improvements to cleanup to make sure files are closed. In addition to bringing in the changes to ExternalAppendOnlyMap, I also copied them to the corresponding code in ExternalSorter and updated its test suite to test for the same issues. Author: Matei Zaharia <[email protected]> Closes #1722 from mateiz/spark-2792 and squashes the following commits: 5d4bfb5 [Matei Zaharia] Make objectStreamReset counter count the last object written too 18fe865 [Matei Zaharia] Update docs on objectStreamReset 576ee83 [Matei Zaharia] Allow objectStreamReset to be 0 0374217 [Matei Zaharia] Remove super paranoid code to close file handles bda37bb [Matei Zaharia] Implement Mridul's ExternalAppendOnlyMap fixes in ExternalSorter too 0d6dad7 [Matei Zaharia] Added Mridul's test changes for ExternalAppendOnlyMap 9a78e4b [Matei Zaharia] Add @mridulm's fixes to ExternalAppendOnlyMap for batch sizes

@mridulm

…in ExternalMap / Sorter All these changes are from mridulm's work in #1609, but extracted here to fix this specific issue and make it easier to merge not 1.1. This particular set of changes is to make sure that we read exactly the right range of bytes from each spill file in EAOM: some serializers can write bytes after the last object (e.g. the TC_RESET flag in Java serialization) and that would confuse the previous code into reading it as part of the next batch. There are also improvements to cleanup to make sure files are closed. In addition to bringing in the changes to ExternalAppendOnlyMap, I also copied them to the corresponding code in ExternalSorter and updated its test suite to test for the same issues. Author: Matei Zaharia <[email protected]> Closes #1722 from mateiz/spark-2792 and squashes the following commits: 5d4bfb5 [Matei Zaharia] Make objectStreamReset counter count the last object written too 18fe865 [Matei Zaharia] Update docs on objectStreamReset 576ee83 [Matei Zaharia] Allow objectStreamReset to be 0 0374217 [Matei Zaharia] Remove super paranoid code to close file handles bda37bb [Matei Zaharia] Implement Mridul's ExternalAppendOnlyMap fixes in ExternalSorter too 0d6dad7 [Matei Zaharia] Added Mridul's test changes for ExternalAppendOnlyMap 9a78e4b [Matei Zaharia] Add @mridulm's fixes to ExternalAppendOnlyMap for batch sizes

andrewor14 · 2014-09-03T23:33:25Z

@mridulm Are the issues in this PR taken care of by #1722 and and #1678? Do we still need this PR?

mridulm · 2014-09-04T01:49:05Z

I think it got split into four issues, two of which got committed, not sure
of the other other two .... And the first one was regressed upon in
1.1.already.
But this or probably is defunct now .... Will close
On 04-Sep-2014 5:03 am, "andrewor14" [email protected] wrote:

@mridulm https://github.com/mridulm Are the issues in this PR taken
care of by #1722 #1722 and and #1678
#1678? Do we still need this PR?

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

…ile consolidation All changes from this PR are by mridulm and are drawn from his work in apache#1609. This patch is intended to fix all major issues related to shuffle file consolidation that mridulm found, while minimizing changes to the code, with the hope that it may be more easily merged into 1.1. This patch is **not** intended as a replacement for apache#1609, which provides many additional benefits, including fixes to ExternalAppendOnlyMap, improvements to DiskBlockObjectWriter's API, and several new unit tests. If it is feasible to merge apache#1609 for the 1.1 deadline, that is a preferable option. Author: Aaron Davidson <[email protected]> Closes apache#1678 from aarondav/consol and squashes the following commits: 53b3f6d [Aaron Davidson] Correct behavior when writing unopened file 701d045 [Aaron Davidson] Rebase with sort-based shuffle 9160149 [Aaron Davidson] SPARK-2532: Minimal shuffle consolidation fixes

@mridulm

…in ExternalMap / Sorter All these changes are from mridulm's work in apache#1609, but extracted here to fix this specific issue and make it easier to merge not 1.1. This particular set of changes is to make sure that we read exactly the right range of bytes from each spill file in EAOM: some serializers can write bytes after the last object (e.g. the TC_RESET flag in Java serialization) and that would confuse the previous code into reading it as part of the next batch. There are also improvements to cleanup to make sure files are closed. In addition to bringing in the changes to ExternalAppendOnlyMap, I also copied them to the corresponding code in ExternalSorter and updated its test suite to test for the same issues. Author: Matei Zaharia <[email protected]> Closes apache#1722 from mateiz/spark-2792 and squashes the following commits: 5d4bfb5 [Matei Zaharia] Make objectStreamReset counter count the last object written too 18fe865 [Matei Zaharia] Update docs on objectStreamReset 576ee83 [Matei Zaharia] Allow objectStreamReset to be 0 0374217 [Matei Zaharia] Remove super paranoid code to close file handles bda37bb [Matei Zaharia] Implement Mridul's ExternalAppendOnlyMap fixes in ExternalSorter too 0d6dad7 [Matei Zaharia] Added Mridul's test changes for ExternalAppendOnlyMap 9a78e4b [Matei Zaharia] Add @mridulm's fixes to ExternalAppendOnlyMap for batch sizes

…h BosonExec enabled (apache#1609) (cherry picked from commit e843d83b83e7351ed3c610a1ac9f016fe5732cb3) Signed-off-by: Dongjoon Hyun <[email protected]>

Consolidated shuffle fixes

f1182f8

Add missing setShutdownStarted hooks

66d6ec3

stylecheck fixes

027c7f1

mateiz reviewed Jul 27, 2014
View reviewed changes

Fix build, add testcases for DiskBlockManagerSuite

195c529

Mridul Muralidharan added 2 commits July 28, 2014 05:20

Consolidated fixes

6095545

Merge branch 'consolidated_shuffle_fixes' of github.com:mridulm/spark…

1c1faea

… into consolidated_shuffle_fixes

witgo reviewed Jul 28, 2014
View reviewed changes

aarondav mentioned this pull request Jul 31, 2014

SPARK-2791: Fix committing, reverting and state tracking in shuffle file consolidation #1678

Closed

mateiz reviewed Jul 31, 2014
View reviewed changes

mateiz mentioned this pull request Aug 1, 2014

SPARK-2792. Fix reading too much or too little data from each stream in ExternalMap / Sorter #1722

Closed

mridulm closed this Sep 4, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-2532] Consolidated shuffle fixes #1609

[SPARK-2532] Consolidated shuffle fixes #1609

mridulm commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

mateiz Jul 27, 2014

mridulm Jul 27, 2014

mateiz commented Jul 27, 2014

SparkQA commented Jul 27, 2014

andrewor14 commented Jul 28, 2014

SparkQA commented Jul 28, 2014

witgo Jul 28, 2014

aarondav commented Jul 28, 2014

aarondav commented Jul 31, 2014

mateiz Jul 31, 2014

mateiz commented Jul 31, 2014

mateiz commented Aug 1, 2014

mateiz commented Aug 1, 2014

andrewor14 commented Sep 3, 2014

mridulm commented Sep 4, 2014

[SPARK-2532] Consolidated shuffle fixes #1609

[SPARK-2532] Consolidated shuffle fixes #1609

Conversation

mridulm commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

SparkQA commented Jul 27, 2014

mateiz Jul 27, 2014

Choose a reason for hiding this comment

mridulm Jul 27, 2014

Choose a reason for hiding this comment

mateiz commented Jul 27, 2014

SparkQA commented Jul 27, 2014

andrewor14 commented Jul 28, 2014

SparkQA commented Jul 28, 2014

witgo Jul 28, 2014

Choose a reason for hiding this comment

aarondav commented Jul 28, 2014

aarondav commented Jul 31, 2014

mateiz Jul 31, 2014

Choose a reason for hiding this comment

mateiz commented Jul 31, 2014

mateiz commented Aug 1, 2014

mateiz commented Aug 1, 2014

andrewor14 commented Sep 3, 2014

mridulm commented Sep 4, 2014