Skip to content

Conversation

@zuston
Copy link
Member

@zuston zuston commented Sep 5, 2022

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
Copy link
Member Author

zuston commented Sep 5, 2022

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

@jerqi
Copy link
Contributor

jerqi commented Sep 5, 2022

The proposal is OK for me.

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2022

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%) ⬇️

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

@jerqi
Copy link
Contributor

jerqi commented Sep 14, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants