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

[#1304] Improvement(rss-client-mr): Using configOption in RssMRConfig

Open myandpr opened this issue 1 year ago • 6 comments

What changes were proposed in this pull request?

as title, Using configOption in RssMRConfig

Why are the changes needed?

Fix: #1304

Does this PR introduce any user-facing change?

No.

How was this patch tested?

existing unit tests

myandpr avatar Jan 09 '24 15:01 myandpr

Codecov Report

Attention: 125 lines in your changes are missing coverage. Please review.

Comparison is base (5d027c0) 53.38% compared to head (1eddea0) 54.68%.

Files Patch % Lines
...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java 0.00% 39 Missing :warning:
...java/org/apache/hadoop/mapreduce/MRClientConf.java 90.49% 22 Missing and 1 partial :warning:
...rg/apache/hadoop/mapred/RssMapOutputCollector.java 0.00% 17 Missing :warning:
...pache/hadoop/mapreduce/task/reduce/RssShuffle.java 0.00% 15 Missing :warning:
...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java 65.78% 13 Missing :warning:
...java/org/apache/uniffle/common/config/RssConf.java 0.00% 13 Missing :warning:
.../org/apache/uniffle/common/config/RssBaseConf.java 0.00% 5 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1427      +/-   ##
============================================
+ Coverage     53.38%   54.68%   +1.30%     
- Complexity     2729     2730       +1     
============================================
  Files           422      402      -20     
  Lines         24046    21935    -2111     
  Branches       2051     2058       +7     
============================================
- Hits          12836    11995     -841     
+ Misses        10412     9213    -1199     
+ Partials        798      727      -71     

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

codecov-commenter avatar Jan 09 '24 16:01 codecov-commenter

Hi @zuston , would you help review this PR when you hava time?

myandpr avatar Jan 09 '24 20:01 myandpr

Hi @jerqi @zuston , PTAL

myandpr avatar Jan 11 '24 07:01 myandpr

Could you help review this? @zhengchenyu

zuston avatar Jan 12 '24 06:01 zuston

I still have my doubts about MRClientConf, same with #1303. I think MRClientConf should not be extended from RssBaseConf, but RssClientConf.

zhengchenyu avatar Jan 12 '24 07:01 zhengchenyu

I still have my doubts about MRClientConf, same with #1303. I think MRClientConf should not be extended from RssBaseConf, but RssClientConf.

RssBaseConf may contain some config options which are dedicated to the server.

jerqi avatar Jan 12 '24 10:01 jerqi