[#2197] Support reg app conf to server and avoid update committed/cached blockIds bitmap #2196

maobaolong · 2024-10-18T04:29:43Z

What changes were proposed in this pull request?

Support reg app conf to server.

Why are the changes needed?

Fix #2197

Avoid update committed/cached blockIds bitmap while storage type is with memory.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Locally

github-actions · 2024-10-18T04:41:41Z

Test Results

2 926 files +1 2 926 suites +1 6h 12m 47s ⏱️ - 1m 15s
1 088 tests ±0 1 086 ✅ +1 2 💤 ±0 0 ❌ - 1
13 630 runs +1 13 600 ✅ +2 30 💤 ±0 0 ❌ - 1

Results for commit 714ef98. ± Comparison against base commit 85b1f01.

♻️ This comment has been updated with latest results.

jerqi · 2024-10-18T07:01:08Z

This may be better as a server configuration.

maobaolong · 2024-10-18T07:27:15Z

@jerqi Done, add a new configuration

codecov-commenter · 2024-10-18T07:35:53Z

Codecov Report

Attention: Patch coverage is 54.54545% with 25 lines in your changes missing coverage. Please review.

Project coverage is 52.97%. Comparing base (4c14b97) to head (d08c314).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
.../apache/uniffle/client/api/ShuffleWriteClient.java	0.00%	6 Missing ⚠️
...ffle/client/request/RssRegisterShuffleRequest.java	0.00%	6 Missing ⚠️
...ava/org/apache/uniffle/server/ShuffleTaskInfo.java	72.22%	2 Missing and 3 partials ⚠️
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java	0.00%	4 Missing ⚠️
...pache/uniffle/server/ShuffleServerGrpcService.java	0.00%	2 Missing ⚠️
...org/apache/uniffle/server/ShuffleFlushManager.java	50.00%	0 Missing and 1 partial ⚠️
.../org/apache/uniffle/server/ShuffleTaskManager.java	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2196      +/-   ##
============================================
+ Coverage     52.31%   52.97%   +0.65%     
- Complexity     2814     3218     +404     
============================================
  Files           452      487      +35     
  Lines         21211    26224    +5013     
  Branches       1950     2477     +527     
============================================
+ Hits          11097    13891    +2794     
- Misses         9405    11422    +2017     
- Partials        709      911     +202

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

jerqi · 2024-10-18T11:11:12Z

Why do we need to register app configuration to server?

jerqi · 2024-10-18T11:13:32Z

proto/src/main/proto/Rss.proto

@@ -193,6 +193,7 @@ message ShuffleRegisterRequest {
  string comparatorClass = 11;
  int32 mergedBlockSize = 12;
  string mergeClassLoader = 13;
+  map<string, string> appConf = 1000;


Prefer clear defining parameters instead of a map.

maobaolong · 2024-10-18T13:10:17Z

@jerqi I plan to use the client side Conf as a app level session config, base on this, we do not need to send BlockLayout for each reportShuffleResult, we only need to get it from app session Conf from ShuffleTaskInfo.

Leverage the appConf, we can easy to extends new features without proto change, even without client upgrade.

jerqi · 2024-10-21T02:31:34Z

@jerqi I plan to use the client side Conf as a app level session config, base on this, we do not need to send BlockLayout for each reportShuffleResult, we only need to get it from app session Conf from ShuffleTaskInfo.

Leverage the appConf, we can easy to extends new features without proto change, even without client upgrade.

I know. But I am no sure that this is a good idea. Application configuration can contain many things. It lacks constraint. We can't get enough information from the interface. It's easy to develop but it's hard to understand.

maobaolong · 2024-10-21T03:27:53Z

@jerqi I see. It make sense about your concern. How about a trade off that we only filter and send the rss config to server by filter the spark.rss prefix?

jerqi · 2024-10-21T03:32:09Z

@jerqi I see. It make sense about your concern. How about a trade off that we only filter and send the rss config to server by filter the spark.rss prefix?

I feel that some options can be passed to the server. But important concepts should be extracted to explict fields in the interface such as BlockLayout

maobaolong · 2024-10-21T04:55:55Z

@jerqi Thanks for your help for improve this PR.

I change to use properties instead of appConf to be more generic to show that we only recommend to pass the common properties to server.
Add a filter of properties to keep the rss client common properties only.
Tested locally, verified there are only 9 items in this properties, it contains no default properties.
Create an new issue to track the Reduce resend BlockLayout improvement [Improvement] Avoid send BlockIdLayout for each GetShuffleResultRequest and GetShuffleResultForMultiPartRequest #2212

maobaolong · 2024-10-22T12:15:53Z

Encountered famous flaky test QuorumTest.case8

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:35)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:179)
	at org.apache.uniffle.test.QuorumTest.case8(QuorumTest.java:829)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)

maobaolong · 2024-10-24T03:19:06Z

ping @jerqi Sorry to remind

jerqi · 2024-10-25T10:00:27Z

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

@@ -719,6 +719,11 @@ public class ShuffleServerConf extends RssBaseConf {
          .booleanType()
          .defaultValue(false)
          .withDescription("Whether to enable app detail log");
+  public static final ConfigOption<Boolean> SERVER_WITH_MEMORY_STORAGE_TYPE_OPTIMIZE_ENABLED =


Could we use camel style for last segment and 4 - 5 segments for the whole config option like
rss.server.storage.bitmapMemoryOptimizeEnabled.

jerqi · 2024-10-25T10:00:58Z

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

@@ -307,4 +318,25 @@ public String toString() {
        + shuffleDetailInfos
        + '}';
  }
+
+  public void setProperties(Map<String, String> properties, ShuffleServerConf serverConf) {


Why do we need serverConf here?

Since I need to check whether this server enabled bitmap optimized feature by SERVER_WITH_MEMORY_STORAGE_TYPE_OPTIMIZE_ENABLED

jerqi · 2024-10-25T10:02:36Z

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

-    synchronized (bitmap) {
+    // With memory storage type should never need cachedBlockIds,
+    // since client do not need call finish shuffle rpc
+    if (!shuffleTaskInfo.isClientStorageTypeWithMemory()) {


We can get this config option from the server configuration directly instead getting from taskInfo.

@jerqi It is different, this is a client side config, it configured by client, so I have to get it from shuffle TaskInfo.

Maybe this config option should be same between server and client side.

maobaolong · 2024-10-28T01:26:22Z

@jerqi Thanks for reply and review, I renamed the property key, I can change to other name if you feel it is not good. PTAL

… cachedBlocksIds bitmaps for with memory client storageType

jerqi · 2024-10-28T03:53:39Z

We would better add check in the commit rpc implement for server side if client uses commit rpc.

jerqi · 2024-10-28T07:36:49Z

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

+      boolean storageTypeWithMemory =
+          StorageType.withMemory(StorageType.valueOf(storageType.name()));
+      if (storageTypeWithMemory) {
+        LOG.warn(


We should throw exception directly.

@jerqi If we throw exception, it will never support for test mode and LOCALFILE client to access the server with LOCALFILE_MEMORY storageType?

I means that it's ok for me to throw exception here.

@jerqi Sorry for the misunderstand, done.

maobaolong · 2024-10-29T04:08:49Z

@jerqi Thanks for remind me, I have misunderstood your suggestion, now I throw exception for these two method, PTAL

jerqi · 2024-10-29T06:27:27Z

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

+                "finishShuffle should not be called while server-side configured StorageType to %s for appId %s",
+                storageType, appId);
+        LOG.error(errorMessage);
+        throw new IllegalStateException(errorMessage);


UnsupportedException?

maobaolong · 2024-10-29T12:41:23Z

@jerqi Thanks for your review!

maobaolong changed the title ~~[#XXXX] Support reg app conf to server and avoid update committed/cached blockIds bitmap~~ [#2197] Support reg app conf to server and avoid update committed/cached blockIds bitmap Oct 18, 2024

maobaolong force-pushed the regAppConfToServer.gh branch from 7342670 to 769372e Compare October 18, 2024 04:43

maobaolong closed this Oct 18, 2024

maobaolong reopened this Oct 18, 2024

maobaolong force-pushed the regAppConfToServer.gh branch from 769372e to ff4b590 Compare October 18, 2024 06:50

maobaolong closed this Oct 18, 2024

maobaolong reopened this Oct 18, 2024

jerqi reviewed Oct 18, 2024

View reviewed changes

maobaolong closed this Oct 21, 2024

maobaolong reopened this Oct 21, 2024

maobaolong force-pushed the regAppConfToServer.gh branch from d08c314 to 58e39ab Compare October 22, 2024 06:17

maobaolong requested a review from jerqi October 22, 2024 06:19

maobaolong force-pushed the regAppConfToServer.gh branch 3 times, most recently from 11328dc to eb87001 Compare October 22, 2024 11:52

maobaolong closed this Oct 22, 2024

maobaolong reopened this Oct 22, 2024

jerqi mentioned this pull request Oct 22, 2024

[MINOR] feat(server): Introduce metrics related bitmaps(committedBlockIds/cachedBlockIds/partitionToBlockIds) #2186

Merged

maobaolong closed this Oct 23, 2024

maobaolong reopened this Oct 23, 2024

maobaolong closed this Oct 23, 2024

maobaolong reopened this Oct 23, 2024

jerqi reviewed Oct 25, 2024

View reviewed changes

maobaolong requested a review from jerqi October 28, 2024 01:24

maobaolong added 3 commits October 28, 2024 11:36

Support reg app conf to server and avoid update committedBlocksId and…

8c266eb

… cachedBlocksIds bitmaps for with memory client storageType

Address comment, rename config key

fb83e78

Address suggestion and use server side configured storage type

dcb37e3

maobaolong force-pushed the regAppConfToServer.gh branch from 199e00d to dcb37e3 Compare October 28, 2024 03:38

Address suggestion check commit/finish shuffle

df22106

maobaolong closed this Oct 28, 2024

maobaolong reopened this Oct 28, 2024

jerqi reviewed Oct 28, 2024

View reviewed changes

Address comments

7317891

jerqi reviewed Oct 29, 2024

View reviewed changes

fix ut

714ef98

jerqi approved these changes Oct 29, 2024

View reviewed changes

maobaolong merged commit fdcb54c into apache:master Oct 29, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2197] Support reg app conf to server and avoid update committed/cached blockIds bitmap #2196

[#2197] Support reg app conf to server and avoid update committed/cached blockIds bitmap #2196

maobaolong commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

jerqi commented Oct 18, 2024

maobaolong commented Oct 18, 2024

codecov-commenter commented Oct 18, 2024 •

edited

Loading

jerqi commented Oct 18, 2024

jerqi Oct 18, 2024

maobaolong commented Oct 18, 2024

jerqi commented Oct 21, 2024

maobaolong commented Oct 21, 2024 •

edited

Loading

jerqi commented Oct 21, 2024

maobaolong commented Oct 21, 2024

maobaolong commented Oct 22, 2024

maobaolong commented Oct 24, 2024

jerqi Oct 25, 2024

maobaolong Oct 28, 2024

jerqi Oct 25, 2024

maobaolong Oct 25, 2024

jerqi Oct 25, 2024

maobaolong Oct 25, 2024

jerqi Oct 28, 2024

maobaolong commented Oct 28, 2024

jerqi commented Oct 28, 2024

jerqi Oct 28, 2024

maobaolong Oct 28, 2024

jerqi Oct 28, 2024

jerqi Oct 29, 2024

maobaolong Oct 29, 2024

maobaolong commented Oct 29, 2024

jerqi Oct 29, 2024

maobaolong commented Oct 29, 2024

[#2197] Support reg app conf to server and avoid update committed/cached blockIds bitmap #2196

[#2197] Support reg app conf to server and avoid update committed/cached blockIds bitmap #2196

Conversation

maobaolong commented Oct 18, 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 Oct 18, 2024 • edited Loading

Test Results

jerqi commented Oct 18, 2024

maobaolong commented Oct 18, 2024

codecov-commenter commented Oct 18, 2024 • edited Loading

Codecov Report

jerqi commented Oct 18, 2024

Choose a reason for hiding this comment

maobaolong commented Oct 18, 2024

jerqi commented Oct 21, 2024

maobaolong commented Oct 21, 2024 • edited Loading

jerqi commented Oct 21, 2024

maobaolong commented Oct 21, 2024

maobaolong commented Oct 22, 2024

maobaolong commented Oct 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong commented Oct 28, 2024

jerqi commented Oct 28, 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

Choose a reason for hiding this comment

maobaolong commented Oct 29, 2024

Choose a reason for hiding this comment

maobaolong commented Oct 29, 2024

maobaolong commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

codecov-commenter commented Oct 18, 2024 •

edited

Loading

maobaolong commented Oct 21, 2024 •

edited

Loading