Skip to content

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

Closed
aarondav wants to merge 3 commits into
apache:masterfrom
aarondav:consol
Closed

SPARK-2791: Fix committing, reverting and state tracking in shuffle file consolidation#1678
aarondav wants to merge 3 commits into
apache:masterfrom
aarondav:consol

Conversation

@aarondav

Copy link
Copy Markdown
Contributor

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.

@SparkQA

Copy link
Copy Markdown

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

@aarondavaarondav mentioned this pull request Jul 31, 2014
5 tasks
@SparkQA

Copy link
Copy Markdown

QA results for PR 1678:
- 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/17535/consoleFull

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.
@SparkQA

Copy link
Copy Markdown

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

@aarondav

Copy link
Copy Markdown
ContributorAuthor

@mridulm Please take a look if possible.

@mateiz This now interacts with the ExternalSorter stuff, and it's possible it partially helps fix a serialization bug (since some serializers apparently write things during a close() after a flush()).

@witgo

Copy link
Copy Markdown
Contributor

We can also merge it to 1.0 branch.

@mateiz

Copy link
Copy Markdown
Contributor

That's true, this is small enough to add back into branch-1.0 too. We should see if we can do the same for the rest of #1609 too (ideally I would like to merge that whole PR into branch-1.0, but I'm not sure how easy that is).

@SparkQA

Copy link
Copy Markdown

QA results for PR 1678:
- 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/17537/consoleFull

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

QA results for PR 1678:
- 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/17553/consoleFull

@aarondav

Copy link
Copy Markdown
ContributorAuthor

Jenkins, retest this please.

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

QA results for PR 1678:
- 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/17565/consoleFull

@aarondav

Copy link
Copy Markdown
ContributorAuthor

Both prior failures are due to being unable to bind to ports, which sounds like leftover state from other jenkins runs (especially since they were two entirely disjoint sets of failed tests related to port binding).

@aarondav

Copy link
Copy Markdown
ContributorAuthor

Jenkins, retest this please.

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

QA results for PR 1678:
- 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/17585/consoleFull

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

revert can throw exception : which will cause other writers to not revert.
We need to wrap it in try/catch, log and continue

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Revert actually doesn't throw, per its (updated) comment.

@mateiz

Copy link
Copy Markdown
Contributor

Jenkins, test this please

@SparkQA

Copy link
Copy Markdown

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

@SparkQA

Copy link
Copy Markdown

QA results for PR 1678:
- 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/17689/consoleFull

@mateiz

Copy link
Copy Markdown
Contributor

@aarondav can you update the name of this PR to reference this sub-task JIRA instead: https://issues.apache.org/jira/browse/SPARK-2791?

@aarondavaarondav changed the title SPARK-2532: Minimal shuffle consolidation fixesSPARK-2791: Fix committing, reverting and state tracking in shuffle file consolidationAug 1, 2014
@aarondav

Copy link
Copy Markdown
ContributorAuthor

Updated title.

@mateiz

Copy link
Copy Markdown
Contributor

Alright, going to merge it then. The changes look good to me.

@asfgitasfgit closed this in 78f2af5Aug 1, 2014
xiliu82 pushed a commit to xiliu82/spark that referenced this pull request Sep 4, 2014
…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 <aaron@databricks.com>
Closesapache#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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This catches Exception but it could catch IOException instead.

sunchao pushed a commit to sunchao/spark that referenced this pull request Jun 2, 2023
Co-authored-by: Russell Spitzer <russell.spitzer@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@aarondav@SparkQA@witgo@mateiz@JoshRosen@mridulm