[#731] feat(spark): Make blockid layout configurable for Spark clients #1528

EnricoMi · 2024-02-14T15:18:23Z

What changes were proposed in this pull request?

Make bit-lengths in block id (block id layout) configurable through dynamic client config from coordinator or client config. Block id layout can be created from RssConf, or where that is not available, is being passed around.

Adds new options (defaults are equivalent to current values in Constants):
- rss.client.blockId.sequenceNoBits
- rss.client.blockId.partitionIdBits
- rss.client.blockId.taskAttemptIdBits
Adds block id layout to two requests (default is layout with current values in Constants).

Default values have moved from Constants into BlockIdLayout. The following replacements exist:

PARTITION_ID_MAX_LENGTH: BlockIdLayout.DEFAULT.partitionIdBits
TASK_ATTEMPT_ID_MAX_LENGTH: BlockIdLayout.DEFAULT.taskAttemptIdBits
ATOMIC_INT_MAX_LENGTH: BlockIdLayout.DEFAULT.sequenceNoBits

Why are the changes needed?

The bit-lengths of sequence number, partition id and task attempt id in block id are defined in common/src/main/java/org/apache/uniffle/common/util/Constants.java. Changing these requires recompiling and redeploying the project. Making this configurable in coordinator.conf, server.conf or client-side would very useful.

Also see #1512, #749.

Fixes #731.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tests.

github-actions · 2024-02-14T15:40:26Z

Test Results

2 326 files + 14 2 326 suites +14 4h 29m 36s ⏱️ - 6m 0s
851 tests + 28 849 ✅ + 27 2 💤 +1 0 ❌ ±0
10 018 runs +321 9 995 ✅ +312 23 💤 +9 0 ❌ ±0

Results for commit bf0902d. ± Comparison against base commit d8aedf3.

♻️ This comment has been updated with latest results.

codecov-commenter · 2024-02-20T10:30:54Z

Codecov Report

Attention: Patch coverage is 67.87879% with 53 lines in your changes are missing coverage. Please review.

Project coverage is 54.60%. Comparing base (9f4ccde) to head (bf0902d).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...pache/uniffle/server/ShuffleServerGrpcService.java	0.00%	17 Missing ⚠️
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java	0.00%	12 Missing ⚠️
.../org/apache/uniffle/common/util/BlockIdLayout.java	79.59%	7 Missing and 3 partials ⚠️
...n/java/org/apache/uniffle/common/util/BlockId.java	55.55%	2 Missing and 2 partials ⚠️
...che/uniffle/client/impl/ShuffleReadClientImpl.java	85.00%	1 Missing and 2 partials ⚠️
...equest/RssGetShuffleResultForMultiPartRequest.java	0.00%	3 Missing ⚠️
...fle/client/request/RssGetShuffleResultRequest.java	0.00%	3 Missing ⚠️
.../org/apache/uniffle/server/ShuffleTaskManager.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1528      +/-   ##
============================================
+ Coverage     53.63%   54.60%   +0.96%     
- Complexity     2822     2837      +15     
============================================
  Files           436      417      -19     
  Lines         24549    22295    -2254     
  Branches       2080     2094      +14     
============================================
- Hits          13167    12174     -993     
+ Misses        10562     9361    -1201     
+ Partials        820      760      -60

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

EnricoMi · 2024-02-26T20:57:39Z

CC @zuston @jerqi @xianjingfeng

EnricoMi · 2024-02-27T06:57:05Z

Should I make Spark2, TEZ and MR clients configurable in this PR as well, our should that better be done in a separate PR?

jerqi · 2024-02-27T07:04:48Z

Should I make Spark2, TEZ and MR clients configurable in this PR as well, our should that better be done in a separate PR?

We can do it in an another pr.

Also moves bit logic from BlockId into BlockIdLayout

EnricoMi · 2024-03-01T12:33:39Z

This is ready for review.

jerqi · 2024-03-03T08:01:44Z

client-tez/src/main/java/org/apache/tez/common/RssTezUtils.java

@@ -53,17 +53,17 @@
 import org.apache.uniffle.client.factory.ShuffleClientFactory;
 import org.apache.uniffle.common.ShuffleServerInfo;
 import org.apache.uniffle.common.exception.RssException;
-import org.apache.uniffle.common.util.BlockId;
+import org.apache.uniffle.common.util.BlockIdLayout;
 import org.apache.uniffle.common.util.Constants;

 public class RssTezUtils {


Could you add more cases for Tez？

Logic is unchanged and layout is not configurable for TEZ, so there is nothing new that needs to be tested.

If existing tests are incomplete, I'd suggest someone who has a deeper understanding of the internals of TEZ task attempt ids than I have adds more coverage in a separate PR. I am happy to adjust them afterwards in this PR. Alternatively, the coverage could be increased when adding configurable block id layout for TEZ in a follow up PR.

Logic is unchanged and layout is not configurable for TEZ, so there is nothing new that needs to be tested.

If existing tests are incomplete, I'd suggest someone who has a deeper understanding of the internals of TEZ task attempt ids than I have adds more coverage in a separate PR. I am happy to adjust them afterwards in this PR. Alternatively, the coverage could be increased when adding configurable block id layout for TEZ in a follow up PR.

OK, I got it . I just feel that you changed the logic of Tez from the title.

jerqi · 2024-03-03T13:54:57Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

@@ -528,6 +529,11 @@ public void getShuffleResult(
    String appId = request.getAppId();
    int shuffleId = request.getShuffleId();
    int partitionId = request.getPartitionId();
+    BlockIdLayout blockIdLayout =


Could you consider some compatible issues? If older client connects a new server, the blockIdLayout may be null.

We could fall back to BlockIdLayout.DEFAULT here, but that would only work for clients that use the original values in Constants.java. For cluster setups where Constants.java has been changed, there is no way to pick the right block id layout here.

request.getBlockIdLayout may return null if we use an old client to connect the shuffle server. The code will throw a NullPointerException.

Yes, what do you suggest? Use BlockIdLayout.DEFAULT if request.getBlockIdLayout == null and mark this a breaking change for users that deployed a cluster with modified bit lengths in Constants.java? How can we make this compatible for those users?

We can use BlockIdLayout.DEFAULT if layout is null. We can't guarantee the compatibility if users modify our code in the project.

I like this, done!

jerqi · 2024-03-03T14:04:23Z

common/src/main/java/org/apache/uniffle/common/segment/FixedSizeSegmentSplitter.java

                  + "This may happen when the data is flushing, please ignore.",
              totalLength,
-              dataFileLen,
-              BlockId.getPartitionId(blockId));


Why do we remove partitionId here?

The actual block id layout is not available here, and wiring it through from the client would mean to add it to many method signature calls. All this only for logging.

At least we should log the long block id here so it can be de-cyphered manually if needed.

…uests

jerqi · 2024-03-05T02:55:36Z

LGTM, @zuston Could you take an another look?

zuston

Looks good overall, left a comment for the compatible problem.

And for the further optimization, the blockIds stored on the shuffle-server for me is not a good design, which will bring too much burden for shuffle-server, especially for many spark jobs, I have raise another issue to track this. #1538

Anyway, the above suggestion is not related with this PR. Thanks for your effort

server/src/main/java/org/apache/uniffle/server/ShuffleServerGrpcService.java

zuston

LGTM. Thanks for your contribution! @EnricoMi

EnricoMi · 2024-03-07T12:42:18Z

What about the SHUFFLE_SERVER_VERSION?

zuston · 2024-03-08T01:05:46Z

What about the SHUFFLE_SERVER_VERSION?

Make sense for me. WDYT? @jerqi

jerqi · 2024-03-08T01:57:03Z

What about the SHUFFLE_SERVER_VERSION?

Make sense for me. WDYT? @jerqi

OK for me.

EnricoMi · 2024-03-08T06:04:10Z

Let me do that in #1566.

EnricoMi mentioned this pull request Feb 14, 2024

[FEATURE] Adaptive length of different parts of blockId #731

Closed

3 tasks

EnricoMi force-pushed the blockid-layout branch 2 times, most recently from 69e87b8 to 0fc6097 Compare February 20, 2024 10:22

EnricoMi force-pushed the blockid-layout branch from 0fc6097 to e3e9a66 Compare February 26, 2024 20:02

EnricoMi marked this pull request as ready for review February 26, 2024 20:02

EnricoMi force-pushed the blockid-layout branch 2 times, most recently from 9fafb5d to e499311 Compare February 26, 2024 20:54

zuston requested review from zhengchenyu and xianjingfeng February 27, 2024 02:12

EnricoMi changed the title ~~[#731] feat(conf): Make blockid layout configurable~~ [#731] feat(conf): Make blockid layout configurable for Spark3 Feb 27, 2024

jerqi changed the title ~~[#731] feat(conf): Make blockid layout configurable for Spark3~~ [#731] feat(spark3): Make blockid layout configurable for Spark3 Feb 28, 2024

EnricoMi force-pushed the blockid-layout branch 2 times, most recently from 1f3e6c7 to 6bd203b Compare February 29, 2024 06:56

jerqi closed this Feb 29, 2024

jerqi reopened this Feb 29, 2024

EnricoMi added 8 commits February 29, 2024 09:14

Move blockId field bit lengths from Constants into BlockIdLayout

b76ee2d

Also moves bit logic from BlockId into BlockIdLayout

Create BlockIdLayout from config, use everywhere

7179361

Use 'Bits' rather than 'Length'

9b5cd42

Add block id config to docs, add spark specific considerations

4c8f5f2

Remove map config type, simplify block id layout config

dac4d97

Fix after rebase

e984f58

Use layout to log block id in ShuffleReadClientImpl

fc5820c

Add tests

7c137d4

EnricoMi force-pushed the blockid-layout branch 2 times, most recently from 3883937 to 262c343 Compare February 29, 2024 13:50

EnricoMi added 3 commits March 1, 2024 07:52

Rename getTaskAttemptId to getTaskAttemptIdForBlockId, fix compile error

6cf1458

Fix code styles

d360754

Better use of parametrized tests

6583ce4

EnricoMi mentioned this pull request Mar 1, 2024

[FEATURE] Support dynamic client conf for creating ShuffleWriteClient #1554

Closed

3 tasks

Test coordinator dynamic client conf

3703c4f

EnricoMi force-pushed the blockid-layout branch from a6a9bec to 3703c4f Compare March 1, 2024 12:13

jerqi reviewed Mar 3, 2024

View reviewed changes

Log block id where partition used to be logged

6700f36

jerqi changed the title ~~[#731] feat(Spark): Make blockid layout configurable for Spark clients~~ [#731] feat(spark): Make blockid layout configurable for Spark clients Mar 4, 2024

EnricoMi added 5 commits March 4, 2024 12:01

Remove default int values, use DEFAULT instance instead

ed7b3b2

Handle and test BlockIdLayout not given in getShuffleResult* GRPC req…

a419387

…uests

Test RssShuffleDataIterator with various layouts

f3bae27

Revert removed empty line

24b153b

Add more details on carefully configuring block id layout for Spark

f1fc15d

EnricoMi force-pushed the blockid-layout branch from 21c70c5 to f1fc15d Compare March 4, 2024 18:00

EnricoMi added 3 commits March 5, 2024 10:24

Fail fast when registering shuffle with unsupported number of partitions

2ab119a

Add more errors to spark client guide blockid section

fe3156b

Move fail-fast test to Spark3

727129e

zuston reviewed Mar 6, 2024

View reviewed changes

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

Remove default int values (complements ed7b3b2)

bf0902d

zuston approved these changes Mar 7, 2024

View reviewed changes

zuston merged commit dc890ff into apache:master Mar 7, 2024
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#731] feat(spark): Make blockid layout configurable for Spark clients #1528

[#731] feat(spark): Make blockid layout configurable for Spark clients #1528

EnricoMi commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

codecov-commenter commented Feb 20, 2024 •

edited

Loading

EnricoMi commented Feb 26, 2024

EnricoMi commented Feb 27, 2024

jerqi commented Feb 27, 2024

EnricoMi commented Mar 1, 2024

jerqi Mar 3, 2024

EnricoMi Mar 3, 2024

jerqi Mar 3, 2024

jerqi Mar 3, 2024

EnricoMi Mar 4, 2024 •

edited

Loading

jerqi Mar 4, 2024 •

edited

Loading

EnricoMi Mar 4, 2024

jerqi Mar 4, 2024

EnricoMi Mar 4, 2024

jerqi Mar 3, 2024

EnricoMi Mar 4, 2024

jerqi Mar 4, 2024

jerqi commented Mar 5, 2024

zuston left a comment •

edited

Loading

zuston left a comment

EnricoMi commented Mar 7, 2024

zuston commented Mar 8, 2024

jerqi commented Mar 8, 2024

EnricoMi commented Mar 8, 2024

[#731] feat(spark): Make blockid layout configurable for Spark clients #1528

[#731] feat(spark): Make blockid layout configurable for Spark clients #1528

Conversation

EnricoMi commented Feb 14, 2024 • 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?

github-actions bot commented Feb 14, 2024 • edited Loading

Test Results

codecov-commenter commented Feb 20, 2024 • edited Loading

Codecov Report

EnricoMi commented Feb 26, 2024

EnricoMi commented Feb 27, 2024

jerqi commented Feb 27, 2024

EnricoMi commented Mar 1, 2024

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

EnricoMi Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

jerqi Mar 4, 2024 • edited Loading

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

jerqi commented Mar 5, 2024

zuston left a comment • edited Loading

Choose a reason for hiding this comment

zuston left a comment

Choose a reason for hiding this comment

EnricoMi commented Mar 7, 2024

zuston commented Mar 8, 2024

jerqi commented Mar 8, 2024

EnricoMi commented Mar 8, 2024

EnricoMi commented Feb 14, 2024 •

edited

Loading

github-actions bot commented Feb 14, 2024 •

edited

Loading

codecov-commenter commented Feb 20, 2024 •

edited

Loading

EnricoMi Mar 4, 2024 •

edited

Loading

jerqi Mar 4, 2024 •

edited

Loading

zuston left a comment •

edited

Loading