[#1373][part-1] feat: partition write to multi servers leveraging from reassignment mechanism #1445

xumanbu · 2024-01-12T05:30:02Z

What changes were proposed in this pull request?

(Please outline the changes and how this PR fixes the issue.)

Why are the changes needed?

Does this PR introduce any user-facing change?

1、add config rss.server.dynamic.assign.enabled for whether to reassign a faulty shuffle server.
2、support reassign a new shuffle server for send failed blocks
3、ShuffleReader read partition in muitl server implement will in next pr

How was this patch tested?

wait follow-up

…aulty server

codecov-commenter · 2024-01-12T05:40:00Z

Codecov Report

Attention: 78 lines in your changes are missing coverage. Please review.

Comparison is base (ca0f0ac) 0.00% compared to head (368dc2b) 55.15%.
Report is 31 commits behind head on master.

Files	Patch %	Lines
...he/uniffle/client/impl/ShuffleWriteClientImpl.java	21.73%	18 Missing ⚠️
...fle/shuffle/manager/ShuffleManagerGrpcService.java	0.00%	14 Missing ⚠️
...request/RssReassignFaultyShuffleServerRequest.java	0.00%	13 Missing ⚠️
...sponse/RssReassignFaultyShuffleServerResponse.java	0.00%	11 Missing ⚠️
...he/uniffle/client/impl/FailedBlockSendTracker.java	52.63%	9 Missing ⚠️
...fle/client/impl/grpc/ShuffleManagerGrpcClient.java	0.00%	5 Missing ⚠️
...pache/uniffle/client/impl/TrackingBlockStatus.java	62.50%	3 Missing ⚠️
...va/org/apache/spark/shuffle/ShuffleHandleInfo.java	0.00%	2 Missing ⚠️
...va/org/apache/spark/shuffle/writer/DataPusher.java	85.71%	0 Missing and 1 partial ⚠️
...uniffle/client/response/SendShuffleDataResult.java	75.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #1445       +/-   ##
=============================================
+ Coverage          0   55.15%   +55.15%     
- Complexity        0     2820     +2820     
=============================================
  Files             0      414      +414     
  Lines             0    22136    +22136     
  Branches          0     2074     +2074     
=============================================
+ Hits              0    12210    +12210     
- Misses            0     9167     +9167     
- Partials          0      759      +759

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xumanbu · 2024-01-12T05:54:27Z

@zuston @jerqi PTAL

zuston · 2024-01-12T06:07:50Z

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

                          + " of concurrent tasks."))
          .createWithDefault(RssClientConfig.RSS_ESTIMATE_SERVER_ASSIGNMENT_ENABLED_DEFAULT_VALUE);

+  public static final ConfigEntry<Boolean> RSS_DYNAMIC_SERVER_ASSIGNMENT_ENABLED =


Please use the config style like this:

public static final ConfigOption<Boolean> RSS_TASK_FAILED_CALLBACK_ENABLED = ConfigOptions.key("rss.task.failed.callback.enable") .booleanType() .defaultValue(true);

zuston · 2024-01-12T06:16:11Z

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

+      int shuffleId, Set<String> partitionIds, String faultyShuffleServerId) {
+    ShuffleServerInfo newShuffleServerInfo;
+    synchronized (this) {
+      if (getReassignedFaultyServers().containsKey(faultyShuffleServerId)) {


How about extract the param of getReassignedFaultyServers

zuston · 2024-01-15T05:48:30Z

proto/src/main/proto/Rss.proto

  repeated string faultyServerIds = 12;
 }

+message ReassignShuffleServerRequest {


Is this unused?

zuston · 2024-01-15T05:51:03Z

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

+    return newShuffleServerInfo;
+  }
+
+  private Map<String, ShuffleServerInfo> reassignedFaultyServers = Maps.newConcurrentMap();


Could you put this var defined in front of the class file ?

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

zuston · 2024-01-15T06:06:12Z

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

+                if (dynamicShuffleServer == null) {
+                  dynamicShuffleServer =
+                      reAssignFaultyShuffleServer(partitionIds, t.getKey().getId());
+                  if (dynamicShuffleServer == null) {


throw exception directly? Otherwise the failed blocks will be ignored.

good catch.

zuston · 2024-01-15T06:12:15Z

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

    }
  }

+  private boolean needReAssignShuffleServer(Set<Long> failedBlockIds) {


I think we should check the blocks existence that meet the requirements, current impl looks wrong.

I got it. I will modify it. thinks.

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

xumanbu · 2024-01-23T06:00:18Z

@jerqi @zuston could you help me take a look again.

xumanbu · 2024-01-25T08:19:14Z

I came up with an overlooked question：reportShuffleResult should rewrite. partitionToBlockIds report to primary server and replica server. but the data structure partitionToBlockIds may not be enough for record blockId -> parititionId-> serverId.

jerqi · 2024-01-29T03:07:26Z

Is this ready for review?

xumanbu · 2024-01-29T06:53:09Z

Is this ready for review?

@zuston tips comments have been corrected. please take a look again.

zuston · 2024-01-31T09:20:44Z

Is this ready for review?

@zuston tips comments have been corrected. please take a look again.

Oh It's a big PR, I will finish review in this week.

zuston

Overall LGTM. Left some comments. Thanks for your effort in advance @xumanbu

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

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

client/src/main/java/org/apache/uniffle/client/util/RssClientConfig.java

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

…llShuffleServersForWholeStage

github-actions · 2024-02-06T08:36:00Z

Test Results

2 429 files 2 429 suites 4h 41m 54s ⏱️
819 tests 818 ✅ 1 💤 0 ❌
9 713 runs 9 699 ✅ 14 💤 0 ❌

Results for commit 368dc2b.

♻️ This comment has been updated with latest results.

zuston

LGTM. Wait 1 day for @jerqi 's review

jerqi · 2024-02-08T11:22:14Z

LGTM. Wait 1 day for @jerqi 's review

I'm will review this pr today.

jerqi · 2024-02-08T17:47:10Z

proto/src/main/proto/Rss.proto

 }
+
+message RssReassignFaultyShuffleServerRequest{
+  int32 shuffleId  = 1;


Should we use int as shuffle id? Maybe we should use this for Tez, too. @zhengchenyu @lifeSo Could you take a look?

In tez, it is ok to use int as shuffle id, and will not over-flow, Here is the shuffle id calc:

private static final int SHUFFLE_ID_MAGIC = 1000; public static int computeShuffleId(int tezDagID, int upVertexId, int downVertexId) { int shuffleId = tezDagID * (SHUFFLE_ID_MAGIC * SHUFFLE_ID_MAGIC) + upVertexId * SHUFFLE_ID_MAGIC + downVertexId; LOG.info("Compute Shuffle Id:{}, up vertex id:{}, down vertex id:{}", shuffleId, upVertexId, downVertexId); return shuffleId; }

Use int as shuffleId following the definitions of other messages.

jerqi · 2024-02-08T17:48:53Z

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

+import org.apache.uniffle.common.ShuffleServerInfo;
+import org.apache.uniffle.common.rpc.StatusCode;
+
+public class TrackBlockStatus {


TrackBlockStatus -> TrackingBlockStatus?

TrackingBlockStatus is better. rename it.

zuston · 2024-02-20T08:21:44Z

cc @jerqi .

zuston · 2024-02-20T08:31:06Z

Do you have any plan to implement the read side change? @xumanbu

xumanbu · 2024-02-20T09:04:48Z

Do you have any plan to implement the read side change? @xumanbu

in progress.I'll submit next PR once ready.

dingshun3016 · 2024-02-21T06:26:41Z

Do you have any plan to implement the read side change? @xumanbu

in progress.I'll submit next PR once ready.

let me have try

xumanbu · 2024-02-21T06:28:31Z

Do you have any plan to implement the read side change? @xumanbu

in progress.I'll submit next PR once ready.

let me have try

it's great.

[apache#1373][part-1] partition write to multi server by reassign a f…

f3440cd

…aulty server

xumanbu mentioned this pull request Jan 12, 2024

[#1373][part-1] add PartitionServerInfo for ShuffleHandleInfo to split partition server #1423

Closed

xumanbu changed the title ~~[#1373][part-1] partition write to multi server by reassign a faulty server~~ [#1373][part-2] partition write to multi server by reassign a faulty server Jan 12, 2024

zuston reviewed Jan 15, 2024

View reviewed changes

xumanbu and others added 19 commits January 18, 2024 10:23

remove ReassignShuffleServerRequest in proto

545c27e

change code style

e7d354b

throw resend failed blocks exception

8dde556

add FailedBlockSendTracker instead of failed blockId and servers

965ca89

opt recordFailedBlocks

4055e34

rename config

955028c

fix code style

f2e83a9

optmized reassignedFaultyServers code

b73a74f

Merge branch 'master' into 1373_mutil_server

160344d

fix bug

cf510fb

fix DataPusher putFailedBlockSendTracker

4edeb2c

fix RssShuffleWriterTest

9da66b1

fix bug

500f927

fix test failed

42fff2c

refactor requestShuffleAssignment

2d3d107

fun needReAssignShuffleServer rename to shouldResendBlockStatusSet

51ebf64

Merge branch 'master' into 1373_mutil_server

779647e

fix spark2 complie fail (not support task fallback)

5f33bba

fix test

e468e29

rename dynamicAssignedPartitionServers to failoverPartitionServers

e2ed4cb

xumanbu force-pushed the 1373_mutil_server branch from b35b34e to f586a24 Compare January 26, 2024 13:17

refactor reportShuffleResult for support partition write to multi server

e2aad86

xumanbu force-pushed the 1373_mutil_server branch from f586a24 to e2aad86 Compare January 29, 2024 06:48

zuston reviewed Feb 4, 2024

View reviewed changes

xumanbu added 3 commits February 6, 2024 15:46

config style fixup

10bb0ae

rename function name to reassignFaultyShuffleServerForTasks&reassignA…

86c5633

…llShuffleServersForWholeStage

FailedBlockSendTracker data struct set to list

baa8534

config move to RssClientConf

8ddcd59

zuston previously approved these changes Feb 8, 2024

View reviewed changes

zuston changed the title ~~[#1373][part-2] partition write to multi server by reassign a faulty server~~ [#1373][part-1] feat: partition write to multi servers leveraging from reassignment mechanism Feb 8, 2024

jerqi reviewed Feb 8, 2024

View reviewed changes

TrackBlockStatus rename to TrackingBlockStatus

368dc2b

xumanbu dismissed zuston’s stale review via 368dc2b February 19, 2024 07:21

zuston approved these changes Feb 20, 2024

View reviewed changes

jerqi approved these changes Feb 20, 2024

View reviewed changes

zuston merged commit 59aa30d into apache:master Feb 21, 2024

zuston mentioned this pull request Mar 29, 2024

Support client partition data reassign #1608

Open

9 tasks

[#1373][part-1] feat: partition write to multi servers leveraging from reassignment mechanism #1445

[#1373][part-1] feat: partition write to multi servers leveraging from reassignment mechanism #1445

Uh oh!

Conversation

xumanbu commented Jan 12, 2024

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?

Uh oh!

codecov-commenter commented Jan 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xumanbu commented Jan 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xumanbu commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xumanbu commented Jan 25, 2024

Uh oh!

jerqi commented Jan 29, 2024

Uh oh!

xumanbu commented Jan 29, 2024

Uh oh!

zuston commented Jan 31, 2024

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

jerqi commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Jan 12, 2024 •

edited

Loading

xumanbu commented Jan 23, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024 •

edited

Loading

lifeSo Feb 18, 2024 •

edited

Loading

dingshun3016 commented Feb 21, 2024 •

edited

Loading