Skip to content

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

Closed
zuston wants to merge 4 commits into
apache:masterfrom
zuston:clientConf
Closed

[WIP] Introduce the general client conf for mr/spark#200
zuston wants to merge 4 commits into
apache:masterfrom
zuston:clientConf

Conversation

@zuston

@zuston zuston commented Sep 5, 2022

Copy link
Copy Markdown
Member

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

zuston commented Sep 5, 2022

Copy link
Copy Markdown
Member Author

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

@roryqi

roryqi commented Sep 5, 2022

Copy link
Copy Markdown
Contributor

The proposal is OK for me.

@codecov-commenter

codecov-commenter commented Sep 5, 2022

Copy link
Copy Markdown

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

@roryqi

roryqi commented Sep 14, 2022

Copy link
Copy Markdown
Contributor

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