Skip to content

[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false #43067

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
wants to merge 1 commit into from
Closed

[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false #43067

wants to merge 1 commit into from

Conversation

@warrenzhu25
Copy link
Contributor

@warrenzhu25 warrenzhu25 commented Sep 23, 2023

What changes were proposed in this pull request?

Add keepReadLock parameter in lockNewBlockForWriting(). When keepReadLock is false, skip lockForReading() to avoid block on read Lock or potential deadlock issue.

When 2 tasks try to compute same rdd with replication level of 2 and running on only 2 executors. Deadlock will happen. Details refer [SPARK-45057]

Task thread hold write lock and waiting for replication to remote executor while shuffle server thread which handling block upload request waiting on lockForReading in BlockInfoManager.scala

Why are the changes needed?

This could save unnecessary read lock acquire and avoid deadlock issue mention above.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added UT in BlockInfoManagerSuite

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the CORE label Sep 23, 2023
mridulm
mridulm approved these changes Sep 23, 2023
View reviewed changes
Copy link
Contributor

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

Thanks for fixing this @warrenzhu25, and nice explanation !
Would be good to get more eyes on this.
+CC @JoshRosen as well.

dongjoon-hyun
dongjoon-hyun reviewed Sep 24, 2023
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.

Could you rebase to master branch once more, @warrenzhu25 ?

@warrenzhu25
Copy link
Contributor Author

warrenzhu25 commented Sep 24, 2023

Could you rebase to master branch once more, @warrenzhu25 ?

Rebased.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 24, 2023

Thank you for updating, @warrenzhu25 .

BTW, GitHub Action seems to lose your running CI link. Could you post the your running GitHub Action link here?

@warrenzhu25
Copy link
Contributor Author

warrenzhu25 commented Sep 24, 2023

Thank you for updating, @warrenzhu25 .

BTW, GitHub Action seems to lose your running CI link. Could you post the your running GitHub Action link here?

Retriggered build.

@warrenzhu25 warrenzhu25 force-pushed the deadlock branch 2 times, most recently from a34518e to 6db6858 Compare September 25, 2023 19:24
@warrenzhu25
Copy link
Contributor Author

warrenzhu25 commented Sep 26, 2023

@JoshRosen Could you help take a look?

JoshRosen
JoshRosen approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@JoshRosen JoshRosen left a comment

Choose a reason for hiding this comment

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

LGTM.

This change looks correct to me and it looks like it preserves the important locking behaviors w.r.t. downstream code: the old code would acquire a read lock only to immediately free it when keepReadLock == false and the new code simply avoids acquiring that lock in the first place.

@mridulm
Copy link
Contributor

mridulm commented Sep 27, 2023

The test failures are unrelated, but can you retrigger them @warrenzhu25 ?
Ideally, would prefer a clean build before merging.

Ngone51
Ngone51 approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@Ngone51 Ngone51 left a comment

Choose a reason for hiding this comment

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

Good catch 👍

@warrenzhu25
Copy link
Contributor Author

warrenzhu25 commented Sep 28, 2023

The test failures are unrelated, but can you retrigger them @warrenzhu25 ? Ideally, would prefer a clean build before merging.

I tried to rebuild several times, but it seems still failing and hanging on LBFGSClusterSuite, but it should be unrelated.

@mridulm mridulm closed this in 0d6fda5 Sep 28, 2023
mridulm pushed a commit that referenced this pull request Sep 28, 2023
@warrenzhu25
[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false
aeba488 
### What changes were proposed in this pull request?
Add `keepReadLock` parameter in `lockNewBlockForWriting()`. When `keepReadLock` is `false`, skip `lockForReading()` to avoid block on read Lock or potential deadlock issue.

When 2 tasks try to compute same rdd with replication level of 2 and running on only 2 executors. Deadlock will happen. Details refer [SPARK-45057]

Task thread hold write lock and waiting for replication to remote executor while shuffle server thread which handling block upload request waiting on `lockForReading` in [BlockInfoManager.scala](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala#L457C24-L457C24)

### Why are the changes needed?
This could save unnecessary read lock acquire and avoid deadlock issue mention above.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Added UT in BlockInfoManagerSuite

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #43067 from warrenzhu25/deadlock.

Authored-by: Warren Zhu <[email protected]>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
(cherry picked from commit 0d6fda5)
Signed-off-by: Mridul Muralidharan <mridulatgmail.com>
mridulm pushed a commit that referenced this pull request Sep 28, 2023
@warrenzhu25
[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false
68db395 
### What changes were proposed in this pull request?
Add `keepReadLock` parameter in `lockNewBlockForWriting()`. When `keepReadLock` is `false`, skip `lockForReading()` to avoid block on read Lock or potential deadlock issue.

When 2 tasks try to compute same rdd with replication level of 2 and running on only 2 executors. Deadlock will happen. Details refer [SPARK-45057]

Task thread hold write lock and waiting for replication to remote executor while shuffle server thread which handling block upload request waiting on `lockForReading` in [BlockInfoManager.scala](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala#L457C24-L457C24)

### Why are the changes needed?
This could save unnecessary read lock acquire and avoid deadlock issue mention above.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Added UT in BlockInfoManagerSuite

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #43067 from warrenzhu25/deadlock.

Authored-by: Warren Zhu <[email protected]>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
(cherry picked from commit 0d6fda5)
Signed-off-by: Mridul Muralidharan <mridulatgmail.com>
mridulm pushed a commit that referenced this pull request Sep 28, 2023
@warrenzhu25
[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false
48138eb 
### What changes were proposed in this pull request?
Add `keepReadLock` parameter in `lockNewBlockForWriting()`. When `keepReadLock` is `false`, skip `lockForReading()` to avoid block on read Lock or potential deadlock issue.

When 2 tasks try to compute same rdd with replication level of 2 and running on only 2 executors. Deadlock will happen. Details refer [SPARK-45057]

Task thread hold write lock and waiting for replication to remote executor while shuffle server thread which handling block upload request waiting on `lockForReading` in [BlockInfoManager.scala](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala#L457C24-L457C24)

### Why are the changes needed?
This could save unnecessary read lock acquire and avoid deadlock issue mention above.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Added UT in BlockInfoManagerSuite

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #43067 from warrenzhu25/deadlock.

Authored-by: Warren Zhu <[email protected]>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
(cherry picked from commit 0d6fda5)
Signed-off-by: Mridul Muralidharan <mridulatgmail.com>
@mridulm
Copy link
Contributor

mridulm commented Sep 28, 2023

Looks like multiple PR's are impacted by it - and in this case, it is not related.
Merging to master, 3.5, 3.4, 3.3

Thanks for fixing this @warrenzhu25 !
Thanks for reviews @JoshRosen, @Ngone51 :-)

viirya pushed a commit to viirya/spark-1 that referenced this pull request Oct 19, 2023
@warrenzhu25 @huaxingao
[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false
fc9ef58 
### What changes were proposed in this pull request?
Add `keepReadLock` parameter in `lockNewBlockForWriting()`. When `keepReadLock` is `false`, skip `lockForReading()` to avoid block on read Lock or potential deadlock issue.

When 2 tasks try to compute same rdd with replication level of 2 and running on only 2 executors. Deadlock will happen. Details refer [SPARK-45057]

Task thread hold write lock and waiting for replication to remote executor while shuffle server thread which handling block upload request waiting on `lockForReading` in [BlockInfoManager.scala](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala#L457C24-L457C24)

### Why are the changes needed?
This could save unnecessary read lock acquire and avoid deadlock issue mention above.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Added UT in BlockInfoManagerSuite

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#43067 from warrenzhu25/deadlock.

Authored-by: Warren Zhu <[email protected]>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
(cherry picked from commit 0d6fda5)
Signed-off-by: Mridul Muralidharan <mridulatgmail.com>
(cherry picked from commit 68db395)
turboFei pushed a commit to turboFei/spark that referenced this pull request Nov 6, 2025
@wangyum @warrenzhu25
[SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false (
37f5a45 
apache#252)

### What changes were proposed in this pull request?
Add `keepReadLock` parameter in `lockNewBlockForWriting()`. When `keepReadLock` is `false`, skip `lockForReading()` to avoid block on read Lock or potential deadlock issue.

When 2 tasks try to compute same rdd with replication level of 2 and running on only 2 executors. Deadlock will happen. Details refer [SPARK-45057]

Task thread hold write lock and waiting for replication to remote executor while shuffle server thread which handling block upload request waiting on `lockForReading` in [BlockInfoManager.scala](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala#L457C24-L457C24)

### Why are the changes needed?
This could save unnecessary read lock acquire and avoid deadlock issue mention above.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Added UT in BlockInfoManagerSuite

### Was this patch authored or co-authored using generative AI tooling?
No

Closes apache#43067 from warrenzhu25/deadlock.

Authored-by: Warren Zhu <[email protected]>
Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>
(cherry picked from commit 0d6fda5)

Co-authored-by: Warren Zhu <[email protected]>
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 left review comments

@JoshRosen JoshRosen JoshRosen approved these changes

@mridulm mridulm mridulm approved these changes

@Ngone51 Ngone51 Ngone51 approved these changes

Assignees

No one assigned

Labels

CORE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@warrenzhu25 @dongjoon-hyun @mridulm @JoshRosen @Ngone51