[ISSUE-468] Put unavailable servers to the end of the list when sending shuffle data #470

xianjingfeng · 2023-01-11T08:41:24Z

What changes were proposed in this pull request?

Put unavailable shuffle servers to the end of the server list when sending shuffle data if replica=1.

Why are the changes needed?

If we use multiple replicas and the first shuffle server becomes unavailable, sending data will take a lot of time. Because the client will always send to the first shuffle server firstly. #468

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

…nding shuffle data

codecov-commenter · 2023-01-11T08:58:04Z

Codecov Report

Merging #470 (bd54e9b) into master (ebaff6a) will increase coverage by 1.17%.
The diff coverage is 79.31%.

@@             Coverage Diff              @@
##             master     #470      +/-   ##
============================================
+ Coverage     58.74%   59.92%   +1.17%     
- Complexity     1664     1785     +121     
============================================
  Files           199      205       +6     
  Lines         11236    11557     +321     
  Branches        999     1043      +44     
============================================
+ Hits           6601     6925     +324     
+ Misses         4243     4226      -17     
- Partials        392      406      +14

Impacted Files	Coverage Δ
...he/uniffle/client/impl/ShuffleWriteClientImpl.java	`35.10% <79.31%> (+5.28%)`	⬆️
...che/uniffle/server/storage/HdfsStorageManager.java	`90.90% <0.00%> (-4.33%)`	⬇️
...he/uniffle/server/storage/MultiStorageManager.java	`60.71% <0.00%> (-3.44%)`	⬇️
...rg/apache/uniffle/server/buffer/ShuffleBuffer.java	`93.38% <0.00%> (-1.66%)`	⬇️
...he/uniffle/server/storage/LocalStorageManager.java	`87.00% <0.00%> (-1.36%)`	⬇️
...ava/org/apache/uniffle/common/util/RetryUtils.java	`71.42% <0.00%> (-1.30%)`	⬇️
...g/apache/hadoop/mapred/SortWriteBufferManager.java	`79.89% <0.00%> (-0.22%)`	⬇️
...ache/uniffle/coordinator/SimpleClusterManager.java	`86.71% <0.00%> (-0.11%)`	⬇️
...pache/uniffle/server/ShuffleServerGrpcService.java	`0.79% <0.00%> (-0.02%)`	⬇️
...he/uniffle/coordinator/CoordinatorGrpcService.java	`2.08% <0.00%> (-0.02%)`	⬇️
... and 23 more

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

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

jerqi · 2023-01-13T02:15:30Z

…nding shuffle data

Could you modify your title and description?

xianjingfeng · 2023-01-13T02:45:17Z

…nding shuffle data

Could you modify your title and description?

Done

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

advancedxy

This generally looks good to me. ping @jerqi to see if he has more input/comments.

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

jerqi · 2023-01-29T11:02:18Z

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

    for (ShuffleServerInfo ssi : serverList) {
+      if (!includeBlockList && replica > 1 && !shuffleServerBlockList.isEmpty()


Will it allocate the servers which is less than replicaNum if exclude nodes are too many?

kaijchen

Thanks @xianjingfeng for the work, I have left some comments, PTAL.

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

kaijchen

LGTM, thanks @xianjingfeng.

jerqi · 2023-01-30T11:26:05Z

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

@@ -105,6 +106,7 @@ public class ShuffleWriteClientImpl implements ShuffleWriteClient {
  private final ExecutorService dataTransferPool;
  private final int unregisterThreadPoolSize;
  private final int unregisterRequestTimeSec;
+  private Set<ShuffleServerInfo> shuffleServerBlocklist;


Why do we use the name shuffleServerBlocklist? What's the meaning of this variable?

Blocklist is equal to Blacklist

Block is a concept of our system. This name make me confused.

Alternatives: disallowlist, denylist, excludelist.
Or maybe just blacklist?

Block is a concept of our system. This name make me confused.

I think so too, but i have not good idea. How about defectiveServerList? We need to unify our opinions. 😂 @jerqi @kaijchen @advancedxy

I think so too, but i have not good idea. How about defectiveServerList? We need to unify our opinions. 😂 @jerqi @kaijchen @advancedxy

Maybe just defectiveServers? Because it's actually a Set.
And withBlocklist may be changed to excludeDefectiveServers.

I think so too, but i have not good idea. How about defectiveServerList? We need to unify our opinions. 😂 @jerqi @kaijchen @advancedxy

Maybe just defectiveServers? Because it's actually a Set. And withBlocklist may be changed to excludeDefectiveServers.

It's ok for me.

+1 for defectiveServers

jerqi · 2023-01-30T11:27:26Z

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java

+    }
+
+    if (assignedNum < replicaNum && withBlocklist) {
+      genServerToBlocks(sbi, serverList, replicaNum - assignedNum,


Do this cause that one shuffle server will be allocated twice?

No. Assigned server will be added to excludeServers.

jerqi

LGTM, thanks @xianjingfeng @advancedxy @kaijchen

xianjingfeng added 2 commits January 11, 2023 16:05

Put unavailable shuffle servers to the end of the server list when se…

b125a86

…nding shuffle data

Optimize

ad45108

jerqi reviewed Jan 11, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

jerqi reviewed Jan 11, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

jerqi reviewed Jan 12, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

Optimize

2b3cd46

xianjingfeng changed the title ~~[ISSUE-468] Put unavailable shuffle servers to the end of the server list when se…~~ [ISSUE-468] Put unavailable servers to the end of the list when sending shuffle data Jan 13, 2023

jerqi reviewed Jan 13, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

Optimize

b19ca06

xianjingfeng requested a review from jerqi January 15, 2023 13:31

advancedxy reviewed Jan 16, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

xianjingfeng added 3 commits January 28, 2023 14:08

Optimize

dfaad93

Optimize

00c1ddf

Fix bug

8aba024

xianjingfeng requested a review from advancedxy January 28, 2023 07:28

advancedxy reviewed Jan 29, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Show resolved Hide resolved

Optimize

9e20527

advancedxy previously approved these changes Jan 29, 2023

View reviewed changes

jerqi reviewed Jan 29, 2023

View reviewed changes

kaijchen reviewed Jan 30, 2023

View reviewed changes

Optimize

a267d70

xianjingfeng dismissed advancedxy’s stale review via a267d70 January 30, 2023 07:32

kaijchen reviewed Jan 30, 2023

View reviewed changes

client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java Outdated Show resolved Hide resolved

Optimize

bd54e9b

kaijchen previously approved these changes Jan 30, 2023

View reviewed changes

jerqi reviewed Jan 30, 2023

View reviewed changes

Optimize

8d6afbb

xianjingfeng dismissed kaijchen’s stale review via 8d6afbb January 31, 2023 08:02

jerqi approved these changes Jan 31, 2023

View reviewed changes

kaijchen approved these changes Jan 31, 2023

View reviewed changes

jerqi linked an issue Jan 31, 2023 that may be closed by this pull request

[Improvement] Put unavailable shuffle servers to the end of the server list when sending shuffle data #468

Closed

3 tasks

jerqi merged commit ebbe2db into apache:master Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE-468] Put unavailable servers to the end of the list when sending shuffle data #470

[ISSUE-468] Put unavailable servers to the end of the list when sending shuffle data #470

xianjingfeng commented Jan 11, 2023 •

edited

codecov-commenter commented Jan 11, 2023 •

edited

jerqi commented Jan 13, 2023

xianjingfeng commented Jan 13, 2023

advancedxy left a comment

jerqi Jan 29, 2023

xianjingfeng Jan 30, 2023

kaijchen left a comment

kaijchen left a comment

jerqi Jan 30, 2023

kaijchen Jan 30, 2023

jerqi Jan 30, 2023 •

edited

kaijchen Jan 30, 2023 •

edited

xianjingfeng Jan 31, 2023

kaijchen Jan 31, 2023 •

edited

jerqi Jan 31, 2023

advancedxy Jan 31, 2023

xianjingfeng Jan 31, 2023

jerqi Jan 30, 2023 •

edited

xianjingfeng Jan 31, 2023

jerqi left a comment

		for (ShuffleServerInfo ssi : serverList) {
		if (!includeBlockList && replica > 1 && !shuffleServerBlockList.isEmpty()

[ISSUE-468] Put unavailable servers to the end of the list when sending shuffle data #470

[ISSUE-468] Put unavailable servers to the end of the list when sending shuffle data #470

Conversation

xianjingfeng commented Jan 11, 2023 • edited

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 Jan 11, 2023 • edited

Codecov Report

jerqi commented Jan 13, 2023

xianjingfeng commented Jan 13, 2023

advancedxy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaijchen left a comment

Choose a reason for hiding this comment

kaijchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Jan 30, 2023 • edited

Choose a reason for hiding this comment

kaijchen Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaijchen Jan 31, 2023 • edited

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

jerqi Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

xianjingfeng commented Jan 11, 2023 •

edited

codecov-commenter commented Jan 11, 2023 •

edited

jerqi Jan 30, 2023 •

edited

kaijchen Jan 30, 2023 •

edited

kaijchen Jan 31, 2023 •

edited

jerqi Jan 30, 2023 •

edited