[Feature] add a configuration to control shuffle data flush #445

advancedxy · 2022-12-22T04:07:59Z

What changes were proposed in this pull request?

Add a configuration to control which kind of shuffle buffers should be flushed

Why are the changes needed?

To reduce potential small I/Os to local persistent storages. This should close #419

Does this PR introduce any user-facing change?

End user: No
Shuffle Server: a new configuration

How was this patch tested?

add UT

zuston · 2022-12-22T12:02:26Z

server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java

-      if (pickedFlushSize > expectedFlushSize) {
-        LOG.info("Finish flush pick with {} bytes", pickedFlushSize);
-        break;
+      if (size > this.shuffleFlushThreshold) {


If max size in pickedShuffle < shuffleFlushThreshold, the flushing mechanism is invalid? Right?

Yeah... This shuffleFlushThreshold is intended to be buffering small I/Os.

If it's a bad configurations, the configurations should be adjusted. Or do you have other ideas?

I think there is no need to do such change. If too much small IO happened, we'd better to expand the cluster to distribute pressure.

Besides, the small IO could be observed by the metrics of event_size_threshold_level4

I think there is no need to do such change. If too much small IO happened, we'd better to expand the cluster to distribute pressure.

How to expand the cluster? more shuffle servers? There would be limited number of shuffle server and you cannot scale to many shuffle servers for one application.

This configurations is specially tailed for HDD only shuffle servers. I think it's necessary to reduce small I/Os to HDDs as much as possible.

event_size_threshold_level4

do you mean event_size_threshold_level1?

Yes. Got your thought. But I consider an extreme case, if the all buffers size are 1M and memory reaches the high-watermark. After that, the later sending data requests are not the existing buffers' partition, whether the memory's data will be flushed?

In this case, will dead-lock happen?

Yes. Got your thought. But I consider an extreme case, if the all buffers size are 1M and memory reaches the high-watermark. After that, the later sending data requests are not the existing buffers' partition, whether the memory's data will be flushed?

In this case, will dead-lock happen?

If misconfigured, say shuffle memory's high watermark: 1G, then all buffer sizes are 1M or less than 1M, then yes, it could be possible all data stays in memory. But like you said, it's an extremely case, and in practice shuffle data must varies differently.

I don't think we need to consider such cases, there are other ways to deal such case: lower down the threshold size for lower high watermark. Do you have other concerns?

Although, It could be a bit more adaptive to select shuffleIds to flush... such as if it doesn't free any memory, then just ignore the threshold and re-select the shuffle ids... But I think it's a bit of overkill/over engineering in this stage to introduce such flexibility.

Emm.. If having this possibility, we should take consideration in our design instead of hoping not happening in users environment.

Emm.. If having this possibility, we should take consideration in our design instead of hoping not happening in users environment.

😂. The option is the safe bet, and the user/admin should take responsibility for what they are configuring. Take the
SERVER_BUFFER_CAPACITY and low/high watermark for analog:

do we need to take considerations when user misconfigured buffer_capacity to some extremely small value, say 1M.

do we need to take considerations when low/high are equal, which makes flush process one shuffle one time

Did you occurred anything similar in your prod envs?

advancedxy · 2022-12-23T08:48:17Z

This is ready for review.

jerqi

LGTM, thanks @advancedxy @zuston

jerqi · 2022-12-26T02:00:08Z

server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java

-        LOG.info("Finish flush pick with {} bytes", pickedFlushSize);
-        break;
+      if (size > this.shuffleFlushThreshold) {
+        pickedFlushSize += size;


What will it happen if shuffle server only have small ios?

shuffle server will not reached to high watermark and thus shuffle data is in almost in memory, which is effectively an in-memory shuffle.

If the shuffle server reached the high watermark, but there only are small I/Os, what will it happen?

I've discussed this situation with @zuston. In practice, the cases you described should not happen.

However since both of you raised similar concern, let me addressed it in this PR.

fixed, please take another look.

Is the behavior different from origin one?

I mean that you change the flush strategy instead of adding an extra configuration. Could we keep the origin behavior by the configuration?

I mean that you change the flush strategy instead of adding an extra configuration. Could we keep the origin behavior by the configuration?

No, the new behavior is controlled by an extra configuration. The origin behavior is not changed by default unless admin/op specifically set the flush threshold.

codecov-commenter · 2022-12-26T03:47:46Z

Codecov Report

Merging #445 (6c1e04a) into master (3aaac3e) will increase coverage by 0.04%.
The diff coverage is 93.10%.

@@             Coverage Diff              @@
##             master     #445      +/-   ##
============================================
+ Coverage     58.65%   58.70%   +0.04%     
- Complexity     1647     1650       +3     
============================================
  Files           199      199              
  Lines         11201    11214      +13     
  Branches        996      998       +2     
============================================
+ Hits           6570     6583      +13     
  Misses         4237     4237              
  Partials        394      394

Impacted Files	Coverage Δ
...he/uniffle/server/buffer/ShuffleBufferManager.java	`82.75% <86.66%> (+0.47%)`	⬆️
...a/org/apache/uniffle/server/ShuffleServerConf.java	`99.27% <100.00%> (+0.01%)`	⬆️

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

jerqi · 2022-12-26T06:16:51Z

LGTM, let @zuston take another look.

advancedxy · 2022-12-27T01:49:34Z

Gently ping @zuston

advancedxy · 2022-12-27T11:23:12Z

Gently ping @zuston

Seems @zuston is busy.. I will wait this PR for tomorrow.

server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java

zuston · 2022-12-28T05:36:31Z

Gently ping @zuston

Seems @zuston is busy.. I will wait this PR for tomorrow.

Sorry, I'm suffering from covid-19 and don't have much time to review this.

advancedxy · 2022-12-28T05:42:38Z

Gently ping @zuston

Seems @zuston is busy.. I will wait this PR for tomorrow.

Sorry, I'm suffering from covid-19 and don't have much time to review this.

Bless and take care of yourself.

jerqi · 2022-12-28T06:10:32Z

Gently ping @zuston

Seems @zuston is busy.. I will wait this PR for tomorrow.

Sorry, I'm suffering from covid-19 and don't have much time to review this.

Bless

zuston reviewed Dec 22, 2022

View reviewed changes

advancedxy marked this pull request as ready for review December 23, 2022 03:40

advancedxy requested a review from jerqi December 23, 2022 08:48

jerqi previously approved these changes Dec 26, 2022

View reviewed changes

jerqi reviewed Dec 26, 2022

View reviewed changes

advancedxy dismissed jerqi’s stale review via f62494d December 26, 2022 03:28

jerqi previously approved these changes Dec 26, 2022

View reviewed changes

advancedxy added 4 commits December 28, 2022 10:15

[Feature] add a configuration to control shuffle data flush

48cb4cf

Add UTs.

f189586

fix style

44a7367

handle rare cases

6c1e04a

advancedxy dismissed jerqi’s stale review via 6c1e04a December 28, 2022 02:15

advancedxy force-pushed the ISSUE_419 branch from f62494d to 6c1e04a Compare December 28, 2022 02:15

jerqi reviewed Dec 28, 2022

View reviewed changes

server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java Show resolved Hide resolved

jerqi approved these changes Dec 28, 2022

View reviewed changes

advancedxy merged commit 6ddf8a7 into apache:master Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] add a configuration to control shuffle data flush #445

[Feature] add a configuration to control shuffle data flush #445

advancedxy commented Dec 22, 2022 •

edited

Loading

zuston Dec 22, 2022

advancedxy Dec 22, 2022

zuston Dec 22, 2022

advancedxy Dec 22, 2022

advancedxy Dec 22, 2022

zuston Dec 23, 2022 •

edited

Loading

advancedxy Dec 23, 2022

advancedxy Dec 23, 2022 •

edited

Loading

zuston Dec 23, 2022

advancedxy Dec 23, 2022 •

edited

Loading

advancedxy commented Dec 23, 2022

jerqi left a comment

jerqi Dec 26, 2022

advancedxy Dec 26, 2022

jerqi Dec 26, 2022

advancedxy Dec 26, 2022

advancedxy Dec 26, 2022

jerqi Dec 28, 2022

jerqi Dec 28, 2022

advancedxy Dec 28, 2022

codecov-commenter commented Dec 26, 2022 •

edited

Loading

jerqi commented Dec 26, 2022

advancedxy commented Dec 27, 2022

advancedxy commented Dec 27, 2022

zuston commented Dec 28, 2022

advancedxy commented Dec 28, 2022

jerqi commented Dec 28, 2022

[Feature] add a configuration to control shuffle data flush #445

[Feature] add a configuration to control shuffle data flush #445

Conversation

advancedxy commented Dec 22, 2022 • 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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuston Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

advancedxy commented Dec 23, 2022

jerqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 26, 2022 • edited Loading

Codecov Report

jerqi commented Dec 26, 2022

advancedxy commented Dec 27, 2022

advancedxy commented Dec 27, 2022

zuston commented Dec 28, 2022

advancedxy commented Dec 28, 2022

jerqi commented Dec 28, 2022

advancedxy commented Dec 22, 2022 •

edited

Loading

zuston Dec 23, 2022 •

edited

Loading

advancedxy Dec 23, 2022 •

edited

Loading

advancedxy Dec 23, 2022 •

edited

Loading

codecov-commenter commented Dec 26, 2022 •

edited

Loading