Skip to content

[#1091] refactor: Refactor the writer code with builder pattern - #1228

Merged
roryqi merged 3 commits into
apache:masterfrom
summaryzb:build_write
Oct 10, 2023
Merged

[#1091] refactor: Refactor the writer code with builder pattern#1228
roryqi merged 3 commits into
apache:masterfrom
summaryzb:build_write

Conversation

@summaryzb

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

As the title

Why are the changes needed?

#1091

Does this PR introduce any user-facing change?

No.

How was this patch tested?

unit test

@codecov-commenter

codecov-commenter commented Oct 9, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1228 (8d622a4) into master (6bfbbc2) will increase coverage by 1.31%.
The diff coverage is 73.56%.

@@             Coverage Diff              @@
##             master    #1228      +/-   ##
============================================
+ Coverage     53.48%   54.79%   +1.31%     
- Complexity     2613     2625      +12     
============================================
  Files           395      375      -20     
  Lines         22584    20288    -2296     
  Branches       1930     1933       +3     
============================================
- Hits          12078    11117     -961     
+ Misses         9771     8501    -1270     
+ Partials        735      670      -65     
Files Coverage Δ
...c/main/java/org/apache/tez/common/RssTezUtils.java 65.66% <100.00%> (+1.70%) ⬆️
...e/uniffle/client/factory/ShuffleClientFactory.java 65.57% <88.63%> (+65.57%) ⬆️
...he/uniffle/client/impl/ShuffleWriteClientImpl.java 35.05% <70.00%> (-0.74%) ⬇️
...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java 38.46% <0.00%> (-2.68%) ⬇️

... and 23 files with indirect coverage changes

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

@roryqi roryqi changed the title improvement: Refactor the writer code with builder pattern [#1091] refactor: Refactor the writer code with builder pattern Oct 10, 2023

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @summaryzb

@roryqi
roryqi merged commit 64c4a68 into apache:master Oct 10, 2023
@summaryzb
summaryzb deleted the build_write branch October 13, 2023 01:00
roryqi pushed a commit that referenced this pull request Jan 2, 2024
### What changes were proposed in this pull request?

Fix a bug that was introduced from [#1228](#1228).
When the RssConf variable is not null, it gets overridden by a new RssConf, resulting in the loss of existing parameters within RssConf.

### Why are the changes needed?

For [#1403](#1403)

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

By setting RSS client configs successfully.
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