[#1554] feat(spark): Fetch dynamic client conf as early as possible

[EnricoMi]

What changes were proposed in this pull request?

Fetches dynamic client config as early as possible, to be able to use dynamic client config to create the shuffle client with updated config.

Why are the changes needed?

Providing config or the shuffle client via coordinator is operationally useful as cluster-wide settings can be deployed through the cluster and changed over time. Clients and apps do not need to change configs.

Fixes Spark part of #1554

Does this PR introduce any user-facing change?

More configs can be provided via coordinators.

How was this patch tested?

Existing and follow-up unit tests.

[github-actions]

Test Results

 2 340 files  ± 0   2 340 suites  ±0   4h 33m 42s ⏱️ + 2m 43s
   908 tests + 4     907 ✅ + 5   1 💤  - 1  0 ❌ ±0 
10 541 runs  +36  10 527 ✅ +45  14 💤  - 9  0 ❌ ±0 

Results for commit 33ac575. ± Comparison against base commit 9737d57.

This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both.

org.apache.uniffle.test.RssShuffleManagerTest ‑ testRssShuffleManagerDynamicClientConf{BlockIdLayout}

org.apache.uniffle.shuffle.manager.RssShuffleManagerBaseTest ‑ testFetchAndApplyDynamicConf
org.apache.uniffle.test.RssShuffleManagerTest ‑ testRssShuffleManagerClientConf{BlockIdLayout}[3]
org.apache.uniffle.test.RssShuffleManagerTest ‑ testRssShuffleManagerDynamicClientConf{BlockIdLayout}[1]
org.apache.uniffle.test.RssShuffleManagerTest ‑ testRssShuffleManagerDynamicClientConf{BlockIdLayout}[2]
org.apache.uniffle.test.RssShuffleManagerTest ‑ testRssShuffleManagerDynamicClientConf{BlockIdLayout}[3]

♻️ This comment has been updated with latest results.

[jerqi]

Why do we need to add this?

[EnricoMi]

This test further down asserts an exception is thrown. Moving RssSparkShuffleUtils.validateRssClientConf(sparkConf) up in the RssShuffleManager constructor throws a different exception if the storage type is not set.

[jerqi]

Coordinator can apply the storage type to the client. Will this pull request affect this feature.

[EnricoMi]

This test calls into new RssShuffleManager(sparkConf, false), which mimics creating the shuffle manager on an executor, and executors do not fetch dynamic config, so the given sparkConf must have the storage type set for the test.

The logic of applying config is unchanged, this only happens earlier. I presume there are tests for this use case, which should check the desired behaviour is preserved.

[zuston]

And I think the PR title is not proper for this change, which don't reflect the feature of overwriting the existing client conf.

[codecov-commenter]

Codecov Report

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

Project coverage is 54.94%. Comparing base (9737d57) to head (33ac575).

Files	Patch %	Lines
...org/apache/spark/shuffle/RssSparkShuffleUtils.java	80.00%	0 Missing and 1 partial ⚠️
...uniffle/shuffle/manager/RssShuffleManagerBase.java	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1557      +/-   ##
============================================
+ Coverage     53.96%   54.94%   +0.98%     
- Complexity     2858     2862       +4     
============================================
  Files           438      418      -20     
  Lines         24793    22455    -2338     
  Branches       2109     2111       +2     
============================================
- Hits          13379    12339    -1040     
+ Misses        10576     9348    -1228     
+ Partials        838      768      -70     

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

[EnricoMi]

... overwriting the existing client conf.

this is not new, this existed before

[zuston]

LGTM.

[zuston]

Could you help resolve the conflict? @EnricoMi

[EnricoMi]

Fixed.