incubator-uniffle icon indicating copy to clipboard operation
incubator-uniffle copied to clipboard

[WIP] Introduce the general client conf for mr/spark

Open zuston opened this issue 3 years ago • 4 comments

This PR is a proposal.

What changes were proposed in this pull request?

Introduce the general client conf for mr/spark

  1. Introduce the class of RssClientConf to set the general configOptions for mr/spark
  2. Introduce the class of RssSparkClientConf to set the spark exclusive configOptions and to construct the RssConf from the SparkConf
  3. Introduce the class of RssMRClientConf

Why are the changes needed?

Now in mr/spark client conf, we don't obey the rule of uniffle configOptions. It causes the some problems as follows

  1. When introducing the client general conf, we have to write three times in RssSparkConfig/ RssClientConfig / RssMRConfig. Actually there is no need to leave dirty works for developers.
  2. If when introducing a general method for MR/Spark and need to some configs from Rss client conf. In current implementation, we have to introduce extra class for method's params. If we having the general RssClientConf, it will benifit more.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UTs.

zuston avatar Sep 05 '22 08:09 zuston

Could u help check this proposal for mr/spark client conf? This POC only does the basic implementation. If this is OK, I will go ahead. @jerqi

zuston avatar Sep 05 '22 08:09 zuston

The proposal is OK for me.

jerqi avatar Sep 05 '22 08:09 jerqi

Codecov Report

Merging #200 (a6c0cc8) into master (331ebb2) will decrease coverage by 0.37%. The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #200      +/-   ##
============================================
- Coverage     58.41%   58.04%   -0.38%     
+ Complexity     1273     1272       -1     
============================================
  Files           158      160       +2     
  Lines          8448     8497      +49     
  Branches        784      787       +3     
============================================
- Hits           4935     4932       -3     
- Misses         3260     3311      +51     
- Partials        253      254       +1     
Impacted Files Coverage Δ
...a/org/apache/spark/shuffle/RssSparkClientConf.java 0.00% <0.00%> (ø)
...rg/apache/uniffle/common/config/RssClientConf.java 0.00% <0.00%> (ø)
...org/apache/uniffle/server/ShuffleFlushManager.java 76.66% <0.00%> (-1.67%) :arrow_down:

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

codecov-commenter avatar Sep 05 '22 08:09 codecov-commenter

Like discussion at dev mail list, we will freeze the code and cut 0.6 version branch in September 15, we will not merge this pr before I cut 0.6 version branch, are you ok?

jerqi avatar Sep 14 '22 10:09 jerqi