[#706] Implement spill method to avoid memory deadlock #714

zuston · 2023-03-13T09:19:11Z

What changes were proposed in this pull request?

Introduce the DataPusher to replace the eventLoop, this could be as general part for spark2 and spark3.
Implement the spill method in WriterBufferManager to avoid memory deadlock.

Why are the changes needed?

In current codebase, if having several WriterBufferManagers, when each other is acquiring memory, the deadlock will happen. To solve this, we should implement spill function to break this deadlock condition.

Fix: #706

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs
Newly added UTs

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

codecov-commenter · 2023-03-13T09:58:08Z

Codecov Report

Merging #714 (d959e35) into master (3f9ba81) will increase coverage by 2.32%.
The diff coverage is 79.66%.

@@             Coverage Diff              @@
##             master     #714      +/-   ##
============================================
+ Coverage     60.95%   63.27%   +2.32%     
- Complexity     1956     1984      +28     
============================================
  Files           244      231      -13     
  Lines         13308    11456    -1852     
  Branches       1119     1125       +6     
============================================
- Hits           8112     7249     -863     
+ Misses         4740     3804     -936     
+ Partials        456      403      -53

Impacted Files	Coverage Δ
...va/org/apache/uniffle/common/util/ThreadUtils.java	`50.00% <44.44%> (-5.56%)`	⬇️
...va/org/apache/spark/shuffle/writer/DataPusher.java	`62.50% <62.50%> (ø)`
...pache/spark/shuffle/writer/WriteBufferManager.java	`85.78% <91.83%> (+2.69%)`	⬆️
.../java/org/apache/spark/shuffle/RssSparkConfig.java	`98.01% <100.00%> (+0.09%)`	⬆️
...org/apache/spark/shuffle/writer/AddBlockEvent.java	`94.11% <100.00%> (+94.11%)`	⬆️

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zuston · 2023-03-14T06:33:14Z

@jerqi PTAL. I have updated the spark3 code, if the design is OK, I will change the spark2 related code.

zuston · 2023-03-14T06:57:38Z

cc @advancedxy @smallzhongfeng

advancedxy · 2023-03-14T09:42:33Z

I did a quick through of the code, I didn't see any serious problem about this problem. I will look it in details later tonight.

However, do you think it's worth a design doc to address problems in #706?

zuston · 2023-03-14T10:03:03Z

However, do you think it's worth a design doc to address problems in #706?

It looks unnecessary, the optimization is not a big change for current codebase.

zuston · 2023-03-14T10:04:04Z

I did a quick through of the code, I didn't see any serious problem about this problem. I will look it in details later tonight.

Looking forward this.

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

advancedxy · 2023-03-14T13:04:19Z

However, do you think it's worth a design doc to address problems in #706?

It looks unnecessary, the optimization is not a big change for current codebase.

OK. Let me do some code review first. Let's decide that later. Even if a design doc is not necessary, some docs might be necessary.

advancedxy

Did some review, this PR may require more work, especially with UTs and some integration tests....

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

client-spark/common/src/test/java/org/apache/spark/shuffle/writer/DataPusherTest.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

zuston · 2023-03-16T03:35:12Z

PTAL again @smallzhongfeng @advancedxy . Thanks

advancedxy · 2023-03-16T07:18:40Z

the check style looks is still failed, please help address that.
I will take a look later.

advancedxy

This version looks in a good shape. I will also review it later this weekend

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/AddBlockEvent.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

jerqi · 2023-03-18T04:15:17Z

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

+import org.apache.uniffle.common.ShuffleBlockInfo;
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public class DataPusher implements Closeable {


Could DataPusher be used for MapReduce?

I haven't seen this part code.

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/common/src/test/java/org/apache/spark/shuffle/writer/WriteBufferManagerTest.java

client-spark/common/src/main/java/org/apache/spark/shuffle/RssSparkConfig.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/AddBlockEvent.java

client-spark/common/src/main/java/org/apache/spark/shuffle/RssSparkConfig.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java

zuston · 2023-03-21T15:35:35Z

PTAL again @advancedxy

advancedxy

Generally lgtm, except some minor comments

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/DataPusher.java

client-spark/common/src/test/java/org/apache/spark/shuffle/writer/WriteBufferManagerTest.java

client-spark/spark2/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

common/src/test/java/org/apache/uniffle/common/util/ThreadUtilsTest.java

common/src/main/java/org/apache/uniffle/common/util/ThreadUtils.java

smallzhongfeng

LGTM except for some minor comment.

zuston · 2023-03-23T01:59:49Z

PTAL again @smallzhongfeng @advancedxy

BTW, this PR has been applied in our internal uniffle, it works well.

zuston · 2023-03-28T03:43:04Z

Just rebase the latest master. cc @advancedxy @smallzhongfeng

advancedxy

LGTM. But it seems that you need to rebase again.

zuston · 2023-03-28T08:58:25Z

LGTM. But it seems that you need to rebase again.

Pity. Add done @advancedxy

…che#714) ### What changes were proposed in this pull request? 1. Introduce the `DataPusher` to replace the `eventLoop`, this could be as general part for spark2 and spark3. 2. Implement the `spill` method in `WriterBufferManager` to avoid memory deadlock. ### Why are the changes needed? In current codebase, if having several `WriterBufferManagers`, when each other is acquiring memory, the deadlock will happen. To solve this, we should implement spill function to break this deadlock condition. Fix: apache#706 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? 1. Existing UTs 2. Newly added UTs

…ock (apache#714)" This reverts commit 1b48c12.

### What changes were proposed in this pull request? Disable the memory spill operation. ### Why are the changes needed? In #714 , the memory spill is introduced to solve the dead lock. For a pity, these part code should be handled carefully, including concurrency and data consistency, like the fix PR #811 . And this part has bugs and I will fix these in the next days. Currently, I want to revert the PR #714. But the partial refactor of #714 is still meaningful. So I submit this PR to disable the memory spill. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Don't need

…sure data correctness (#1558) ### What changes were proposed in this pull request? Verify the number of written records to enhance data accuracy. Make sure all data records are sent by clients. Make sure bugs like #714 will never be introduced into the code. ### Why are the changes needed? A follow-up PR for #848. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing UTs.

zuston marked this pull request as draft March 13, 2023 09:19

zuston commented Mar 13, 2023

View reviewed changes

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java Show resolved Hide resolved

zuston marked this pull request as ready for review March 14, 2023 06:32

zuston requested review from jerqi, advancedxy and smallzhongfeng March 14, 2023 06:37

zuston mentioned this pull request Mar 14, 2023

[Improvement] Fine-grained memory control for spark client #706

Closed

3 tasks

smallzhongfeng reviewed Mar 14, 2023

View reviewed changes

advancedxy reviewed Mar 14, 2023

View reviewed changes

zuston requested review from advancedxy and smallzhongfeng March 15, 2023 13:08

zuston force-pushed the issue-706 branch from e0b2f29 to b12cf46 Compare March 16, 2023 03:33

advancedxy reviewed Mar 17, 2023

View reviewed changes

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/AddBlockEvent.java Show resolved Hide resolved

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java Show resolved Hide resolved

jerqi reviewed Mar 17, 2023

View reviewed changes

client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java Show resolved Hide resolved

jerqi reviewed Mar 18, 2023

View reviewed changes

advancedxy reviewed Mar 19, 2023

View reviewed changes

zuston force-pushed the issue-706 branch from 82b6e20 to 338b6e8 Compare March 20, 2023 06:23

advancedxy reviewed Mar 20, 2023

View reviewed changes

zuston requested a review from advancedxy March 20, 2023 11:25

advancedxy reviewed Mar 22, 2023

View reviewed changes

smallzhongfeng reviewed Mar 22, 2023

View reviewed changes

common/src/test/java/org/apache/uniffle/common/util/ThreadUtilsTest.java Show resolved Hide resolved

common/src/main/java/org/apache/uniffle/common/util/ThreadUtils.java Show resolved Hide resolved

smallzhongfeng reviewed Mar 22, 2023

View reviewed changes

zuston requested review from advancedxy and smallzhongfeng March 22, 2023 11:16

advancedxy previously approved these changes Mar 27, 2023

View reviewed changes

zuston dismissed advancedxy’s stale review via fbeb6b9 March 28, 2023 03:30

zuston force-pushed the issue-706 branch from 3eb0ff6 to fbeb6b9 Compare March 28, 2023 03:30

zuston requested a review from advancedxy March 28, 2023 06:52

advancedxy previously approved these changes Mar 28, 2023

View reviewed changes

zuston added 2 commits March 28, 2023 16:28

[apache#706] Implement spill method to avoid memory preemption

a0fa0b1

checkstyle fix

d959e35

zuston dismissed advancedxy’s stale review via d959e35 March 28, 2023 08:28

zuston force-pushed the issue-706 branch from 8bd3e19 to d959e35 Compare March 28, 2023 08:28

zuston requested a review from advancedxy March 28, 2023 08:29

fix

dddc066

advancedxy approved these changes Mar 28, 2023

View reviewed changes

zuston merged commit 1b48c12 into apache:master Mar 28, 2023

jerqi added a commit to jerqi/incubator-uniffle that referenced this pull request Apr 11, 2023

Revert "[apache#706] feat(spark): support spill to avoid memory deadl…

82364b1

…ock (apache#714)" This reverts commit 1b48c12.

jerqi added a commit to jerqi/incubator-uniffle that referenced this pull request Apr 11, 2023

Revert "[apache#706] feat(spark): support spill to avoid memory deadl…

e73d339

…ock (apache#714)" This reverts commit 1b48c12.

zuston mentioned this pull request Apr 26, 2023

fix(client): disable spark memory spill #844

Merged

rickyma mentioned this pull request Mar 6, 2024

[#808] improvement(spark): Verify the number of written records to enhance data correctness #1558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#706] Implement spill method to avoid memory deadlock #714

[#706] Implement spill method to avoid memory deadlock #714

zuston commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 13, 2023 •

edited

Loading

zuston commented Mar 14, 2023

zuston commented Mar 14, 2023 •

edited

Loading

advancedxy commented Mar 14, 2023

zuston commented Mar 14, 2023

zuston commented Mar 14, 2023

advancedxy commented Mar 14, 2023

advancedxy left a comment

zuston commented Mar 16, 2023

advancedxy commented Mar 16, 2023

advancedxy left a comment

jerqi Mar 18, 2023

zuston Mar 20, 2023

zuston commented Mar 21, 2023

advancedxy left a comment

smallzhongfeng left a comment

zuston commented Mar 23, 2023 •

edited

Loading

zuston commented Mar 28, 2023

advancedxy left a comment

zuston commented Mar 28, 2023

[#706] Implement spill method to avoid memory deadlock #714

[#706] Implement spill method to avoid memory deadlock #714

Conversation

zuston commented Mar 13, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov-commenter commented Mar 13, 2023 • edited Loading

Codecov Report

zuston commented Mar 14, 2023

zuston commented Mar 14, 2023 • edited Loading

advancedxy commented Mar 14, 2023

zuston commented Mar 14, 2023

zuston commented Mar 14, 2023

advancedxy commented Mar 14, 2023

advancedxy left a comment

Choose a reason for hiding this comment

zuston commented Mar 16, 2023

advancedxy commented Mar 16, 2023

advancedxy left a comment

Choose a reason for hiding this comment

jerqi Mar 18, 2023

Choose a reason for hiding this comment

zuston Mar 20, 2023

Choose a reason for hiding this comment

zuston commented Mar 21, 2023

advancedxy left a comment

Choose a reason for hiding this comment

smallzhongfeng left a comment

Choose a reason for hiding this comment

zuston commented Mar 23, 2023 • edited Loading

zuston commented Mar 28, 2023

advancedxy left a comment

Choose a reason for hiding this comment

zuston commented Mar 28, 2023

zuston commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 13, 2023 •

edited

Loading

zuston commented Mar 14, 2023 •

edited

Loading

zuston commented Mar 23, 2023 •

edited

Loading