Skip to content

[SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received #39316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
mridulm wants to merge 3 commits into from
Closed

[SPARK-41792][Shuffle] Fix DB update for push based shuffle when newer shuffle merge is received #39316

mridulm wants to merge 3 commits into from

Conversation

@mridulm
Copy link
Contributor

@mridulm mridulm commented Dec 30, 2022

What changes were proposed in this pull request?

Incorrect merge id is removed from the DB when a newer shuffle merge id is received.

Why are the changes needed?

Bug fix

Does this PR introduce any user-facing change?

No, fixes a corner case bug

How was this patch tested?

Unit test updated

@github-actions github-actions bot added the CORE label Dec 30, 2022
@mridulm
Copy link
Contributor Author

mridulm commented Dec 30, 2022 •
edited
Loading

+CC @zhouyejoe, @otterc, @Ngone51

@mridulm
Copy link
Contributor Author

mridulm commented Dec 30, 2022

+CC @wankunde, this fixes the bug I had described earlier.

dongjoon-hyun
dongjoon-hyun reviewed Dec 31, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

According to the JIRA, is this for both Apache Spark 3.3.2 and 3.4.0, @mridulm ?

dongjoon-hyun
dongjoon-hyun approved these changes Dec 31, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

+1, LGTM. (BTW, nit, the Java-style 8-space indentation for continued lines looks a little strange to me. Since we used 2-space indentation even in Java, I thought we used 4-space for continued lines)

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Dec 31, 2022

Oh, could you take a look at the failures? It looks like relevant.

[info] *** 2 TESTS FAILED ***
[error] Failed tests:
[error] 	org.apache.spark.network.yarn.YarnShuffleServiceWithRocksDBBackendSuite
[error] 	org.apache.spark.network.yarn.YarnShuffleServiceWithLevelDBBackendSuite

@wankunde
Copy link
Contributor

wankunde commented Dec 31, 2022

+1, LGTM

zhouyejoe
zhouyejoe approved these changes Dec 31, 2022
View reviewed changes
Copy link
Contributor

@zhouyejoe zhouyejoe left a comment

Choose a reason for hiding this comment

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

+1 LGTM

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java Outdated
closeAndDeleteOutdatedPartitions(
appAttemptShuffleMergeId, mergePartitionsInfo.shuffleMergePartitions));
closeAndDeleteOutdatedPartitions(currrentAppAttemptShuffleMergeId,
mergePartitionsInfo.shuffleMergePartitions));
Copy link
Contributor

@zhouyejoe zhouyejoe Dec 31, 2022

Choose a reason for hiding this comment

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

Nit: 4 spaces for continued lines.

@yabola
Copy link
Contributor

yabola commented Jan 1, 2023 •
edited
Loading

@mridulm
I agree with you. The failed UT is as expected and we should modify the UT.
Actually my previous PR also remove the current mergeId and modify the UT https://github.com/apache/spark/pull/38560/files#r1031350719

In my previous PR, I tried unify this deletion logic: delete mergePartitionsInfo.shuffleMergePartitions at the same time deleting the DB. What more, I also want to unify the deleting the finalized partition information ( if higher mergeId come and merge partitions was finalized, we should clean finalized partitions, please see here https://github.com/apache/spark/pull/38560/files#diff-d544fbb952b61283b3d18ca10a5027528efc4f2f65047130da015ae7754c117fR502)

@mridulm
Copy link
Contributor Author

mridulm commented Jan 1, 2023 •
edited
Loading

Let me take a look, thanks @dongjoon-hyun.
Did not notice this failing in my local box ... weird. I switched branches after the specific test I modified was done and restarted build :-(

@mridulm
Copy link
Contributor Author

mridulm commented Jan 1, 2023

Agree with @yabola, the test is broken: I will fix the test to the expected output.

@github-actions github-actions bot added the YARN label Jan 1, 2023
@mridulm
Copy link
Contributor Author

mridulm commented Jan 1, 2023

@dongjoon-hyun:

According to the JIRA, is this for both Apache Spark 3.3.2 and 3.4.0, @mridulm ?

This is only for master, I have updated the jira ...

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 1, 2023

Got it. Thank you, @mridulm

@mridulm
Copy link
Contributor Author

mridulm commented Jan 1, 2023

Merging to master

@mridulm mridulm closed this in 5d6b69f Jan 1, 2023
@mridulm
Copy link
Contributor Author

mridulm commented Jan 1, 2023

Thanks for the review @dongjoon-hyun, @wankunde, @zhouyejoe, @yabola :-)

@mridulm mridulm mentioned this pull request Jan 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

+1 more reviewer

@zhouyejoe zhouyejoe zhouyejoe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CORE YARN

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mridulm @dongjoon-hyun @wankunde @yabola @zhouyejoe