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

[maobaolong]

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[bot]]

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 714ef98e. ± Comparison against base commit 85b1f01e.

:recycle: This comment has been updated with latest results.

[jerqi]

This may be better as a server configuration.

[maobaolong]

@jerqi Done, add a new configuration

[codecov-commenter]

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 :warning:
...ffle/client/request/RssRegisterShuffleRequest.java	0.00%	6 Missing :warning:
...ava/org/apache/uniffle/server/ShuffleTaskInfo.java	72.22%	2 Missing and 3 partials :warning:
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java	0.00%	4 Missing :warning:
...pache/uniffle/server/ShuffleServerGrpcService.java	0.00%	2 Missing :warning:
...org/apache/uniffle/server/ShuffleFlushManager.java	50.00%	0 Missing and 1 partial :warning:
.../org/apache/uniffle/server/ShuffleTaskManager.java	91.66%	0 Missing and 1 partial :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jerqi]

Why do we need to register app configuration to server?

[maobaolong]

@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]

@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]

@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]

@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]

@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 https://github.com/apache/incubator-uniffle/issues/2212

[maobaolong]

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]

ping @jerqi Sorry to remind

[maobaolong]

@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

[jerqi]

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

[maobaolong]

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

[maobaolong]

@jerqi Thanks for your review!