[#825][part-3] feat(spark): Get the ShuffleServer corresponding to the partition from ShuffleManager by yl09099 · Pull Request #1141 · apache/uniffle

yl09099 · 2023-08-13T07:45:11Z

What changes were proposed in this pull request?

ShuffleReader and ShuffleWriter get the ShuffleServer corresponding to the partition from ShuffleManager

Ⅰ. Overall objective:

During the shuffle write phase, the ShuffleServer reports faulty nodes and reallocates the ShuffleServer list;
Triggers a Stage level retry of SPARK. The shuffleServer node is excluded and reallocated before the retry.

Ⅱ. Implementation logic diagram:

Ⅲ. As shown in the picture above:

During Shuffle registration, obtain the ShuffleServer list to be written through the RPC interface of a Coordinator Client by following the solid blue line step. The list is bound using ShuffleID.
2, the Task of Stage starts, solid steps, in accordance with the green by ShuffleManager Client RPC interface gets to be written for shuffleIdToShuffleHandleInfo ShuffleServer list;
In the Stage, if Task fails to write blocks to the ShuffleServer, press the steps in red to report ShuffleServer to FailedShuffleServerList in RSSShuffleManager through the RPC interface.
FailedShuffleServerList records the number of ShuffleServer failures. After the number of failures reaches the maximum number of retries of the Task level, follow the steps in dotted orange lines. Through the RPC interface of a Coordinator Client, obtain the list of ShuffleServer files to be written (the ShuffleServer files that fail to be written are excluded). After obtaining the list, go to Step 5 of the dotted orange line. Throwing a FetchFailed Exception triggers a stage-level retry for SPARK;
Attempt 1 is generated by the SPARK Stage level again. Pull the corresponding ShuffleServer list according to the green dotted line.

Why are the changes needed?

Fix: #825

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UT.

codecov-commenter · 2023-08-13T08:08:20Z

Codecov Report

Merging #1141 (87ec4b6) into master (2808d25) will increase coverage by 0.89%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1141      +/-   ##
============================================
+ Coverage     53.94%   54.83%   +0.89%     
+ Complexity     2655     2653       -2     
============================================
  Files           396      378      -18     
  Lines         23057    20797    -2260     
  Branches       1971     1976       +5     
============================================
- Hits          12437    11405    -1032     
+ Misses         9854     8694    -1160     
+ Partials        766      698      -68

Files	Coverage Δ
...fle/client/impl/grpc/ShuffleManagerGrpcClient.java	`38.46% <0.00%> (-11.54%)`	⬇️
...nt/request/RssPartitionToShuffleServerRequest.java	`0.00% <0.00%> (ø)`
...a/org/apache/uniffle/common/ShuffleServerInfo.java	`63.82% <0.00%> (-32.95%)`	⬇️
...fle/shuffle/manager/ShuffleManagerGrpcService.java	`57.52% <0.00%> (-24.76%)`	⬇️
.../response/RssPartitionToShuffleServerResponse.java	`0.00% <0.00%> (ø)`

... and 23 files with indirect coverage changes

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

roryqi · 2023-10-09T12:16:09Z

@leixm Could you help review this pr first?

leixm · 2023-10-10T07:39:17Z

@leixm Could you help review this pr first?

Sure.

leixm · 2023-10-16T11:38:06Z

+      RssShuffleHandle<K, V, C> rssHandle,
+      ShuffleHandleInfo shuffleHandleInfo,
+      TaskContext context,
+      ShuffleManagerClient shuffleManagerClient) {


@yl09099 Why change this constructor to add TaskContext and ShuffleManagerClient?

Is it possible to put shuffleHandleInfo into rssHandle? This will make the code cleaner. @yl09099

@yl09099 Why change this constructor to add TaskContext and ShuffleManagerClient?

TaskContext

ShuffleManagerClient has been removed, from which TaskContext's later PR needs to obtain information such as Stage AttemptNumber.

Is it possible to put shuffleHandleInfo into rssHandle? This will make the code cleaner. @yl09099

To put shuffleHandleInfo object into the broadcast on the Driver side, if you want to put shuffleHandleInfo into rssHandle, you need to build RPC interface through the Driver's broadcast object to put in, here I think it is a waste of performance.

leixm · 2023-10-16T11:40:59Z

    this.hadoopConf = hadoopConf;
    this.readMetrics = readMetrics;
-    this.partitionToShuffleServers = rssShuffleHandle.getPartitionToServers();
+    this.partitionToShuffleServers = allPartitionToServers;


Here too, can we put the information maintained by ShuffleManagerService into rssShuffleHandle, so that it is transparent to RssShuffleReader and RssShuffleWriter.

roryqi · 2023-10-23T03:07:48Z

+  }
+
+  private ShuffleManagerClient createShuffleManagerClient(String host, int port) {
+    // host is passed from spark.driver.bindAddress, which would be set when SparkContext is


Host can be inferred from `spark.driver.bindAddress`

Host can be inferred from `spark.driver.bindAddress`

Have been modified.

roryqi · 2023-10-23T03:53:46Z

+   * Mapping between ShuffleId and ShuffleServer list. ShuffleServer list is dynamically allocated.
+   * ShuffleServer is not obtained from RssShuffleHandle, but from this mapping.
+   */
+  private Map<Integer, ShuffleHandleInfo> shuffleIdToShuffleHandleInfo = Maps.newConcurrentMap();


Should we use JavaUtils.newConcurrentMap?

Should we use JavaUtils.newConcurrentMap?

Have been modified.

roryqi · 2023-10-23T03:53:54Z

@@ -107,6 +114,14 @@ public class RssShuffleManager extends RssShuffleManagerBase {
  private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();


roryqi · 2023-10-23T03:54:18Z

            failedTaskIds,
            poolSize,
            keepAliveTime);
+    this.shuffleIdToShuffleHandleInfo = Maps.newConcurrentMap();


roryqi · 2023-10-23T03:54:56Z

LGTM, some minor comments. Let @leixm take an another look.

zuston · 2023-10-30T05:59:01Z

Is this ready to merge? @jerqi .

roryqi · 2023-10-30T06:10:24Z

Is this ready to merge? @jerqi .

let @leixm take another look, and let @leixm merge this pr.

leixm · 2023-10-30T09:02:43Z

LGTM, waiting for UTs.

… to the partition from ShuffleManager.

leixm · 2023-10-30T11:09:39Z

Merged, thanks @jerqi @zuston @yl09099

yl09099 changed the title ~~[#825][part-2] feat(spark): Get the ShuffleServer corresponding to the partition from ShuffleManager~~ [#825][part-3] feat(spark): Get the ShuffleServer corresponding to the partition from ShuffleManager Aug 13, 2023

roryqi requested a review from advancedxy August 13, 2023 07:59

yl09099 force-pushed the uniffle-825-3 branch 4 times, most recently from 3743d7f to 59eaf77 Compare August 14, 2023 02:07

yl09099 closed this Oct 9, 2023

yl09099 force-pushed the uniffle-825-3 branch from 59eaf77 to 1406e1f Compare October 9, 2023 02:11

yl09099 reopened this Oct 9, 2023

yl09099 force-pushed the uniffle-825-3 branch 2 times, most recently from d41d93f to 2b2cac0 Compare October 9, 2023 11:43

roryqi requested a review from leixm October 9, 2023 12:16

leixm requested changes Oct 16, 2023

View reviewed changes

yl09099 closed this Oct 23, 2023

yl09099 force-pushed the uniffle-825-3 branch from 34cb050 to 6bfbbc2 Compare October 23, 2023 01:55

yl09099 reopened this Oct 23, 2023

yl09099 force-pushed the uniffle-825-3 branch from 3e12c99 to d076845 Compare October 23, 2023 02:50

roryqi reviewed Oct 23, 2023

View reviewed changes

yl09099 force-pushed the uniffle-825-3 branch 2 times, most recently from 3c50aa8 to 21ffc28 Compare October 23, 2023 03:28

roryqi reviewed Oct 23, 2023

View reviewed changes

yl09099 force-pushed the uniffle-825-3 branch from 21ffc28 to 448c4c2 Compare October 23, 2023 04:51

leixm requested changes Oct 30, 2023

View reviewed changes

Comment thread ...spark/common/src/main/java/org/apache/uniffle/shuffle/manager/ShuffleManagerGrpcService.java

yl09099 closed this Oct 30, 2023

yl09099 force-pushed the uniffle-825-3 branch from 448c4c2 to 87ee4d4 Compare October 30, 2023 08:54

yl09099 reopened this Oct 30, 2023

leixm previously approved these changes Oct 30, 2023

View reviewed changes

yl09099 dismissed leixm’s stale review via 409b8f9 October 30, 2023 09:05

yl09099 force-pushed the uniffle-825-3 branch 5 times, most recently from 6790789 to fd22e82 Compare October 30, 2023 09:27

[apache#825][part-3] feat(spark): Get the ShuffleServer corresponding…

4a57f5c

… to the partition from ShuffleManager.

yl09099 force-pushed the uniffle-825-3 branch from fd22e82 to 4a57f5c Compare October 30, 2023 09:29

leixm approved these changes Oct 30, 2023

View reviewed changes

leixm merged commit e9a62ee into apache:master Oct 30, 2023

		@@ -107,6 +114,14 @@ public class RssShuffleManager extends RssShuffleManagerBase {
		private final Map<Integer, Integer> shuffleIdToNumMapTasks = Maps.newConcurrentMap();

Uh oh!

Conversation

yl09099 commented Aug 13, 2023

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 Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

roryqi commented Oct 9, 2023

Uh oh!

leixm commented Oct 10, 2023

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!

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!

roryqi commented Oct 23, 2023

Uh oh!

zuston commented Oct 30, 2023

Uh oh!

roryqi commented Oct 30, 2023

Uh oh!

Uh oh!

leixm commented Oct 30, 2023

Uh oh!

leixm commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 13, 2023 •

edited

Loading