Skip to content

Conversation

@EnricoMi
Copy link
Contributor

@EnricoMi EnricoMi commented Feb 14, 2024

What changes were proposed in this pull request?

Moves block id bit manipulation logic into one place in common.util. Logic is tested once and reused everywhere. Reused in RssTezUtils and RssMRUtils where reasonable. Aligns the order of arguments that constructs a BlockId with the bit-wise order inside the block id (highest to lowest).

Why are the changes needed?

Reduces code duplication and improves code readability.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests.

@EnricoMi EnricoMi changed the title [MINOR] refactor(common): Move blockId bit logic into common clas [MINOR] refactor(common): Move blockId bit logic into common class Feb 14, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2024

Codecov Report

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

Comparison is base (ff555c5) 54.15% compared to head (a134098) 55.20%.

Files Patch % Lines
...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java 91.66% 0 Missing and 1 partial ⚠️
...c/main/java/org/apache/tez/common/RssTezUtils.java 88.88% 0 Missing and 1 partial ⚠️
...che/uniffle/client/impl/ShuffleReadClientImpl.java 50.00% 1 Missing ⚠️
...niffle/storage/common/FileBasedShuffleSegment.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1527      +/-   ##
============================================
+ Coverage     54.15%   55.20%   +1.04%     
- Complexity     2805     2816      +11     
============================================
  Files           431      412      -19     
  Lines         24421    22068    -2353     
  Branches       2081     2077       -4     
============================================
- Hits          13226    12182    -1044     
+ Misses        10363     9132    -1231     
+ Partials        832      754      -78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Feb 14, 2024

Test Results

2 452 files  +14  2 452 suites  +14   4h 42m 8s ⏱️ -22s
  823 tests ± 0    822 ✅ ± 0   1 💤 ±0  0 ❌ ±0 
9 743 runs  ± 0  9 729 ✅ ± 0  14 💤 ±0  0 ❌ ±0 

Results for commit a134098. ± Comparison against base commit ff555c5.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
org.apache.uniffle.client.ClientUtilsTest ‑ getBlockIdTest
org.apache.uniffle.common.util.BlockIdTest ‑ test

♻️ This comment has been updated with latest results.

@EnricoMi
Copy link
Contributor Author

@zuston @jerqi what do you think?

@EnricoMi EnricoMi force-pushed the blockid-bit-logic branch 2 times, most recently from 4876aac to da464d8 Compare February 16, 2024 07:56
@zuston zuston requested a review from jerqi February 18, 2024 02:30
@zuston
Copy link
Member

zuston commented Feb 20, 2024

Could you help review this? @jerqi

Moves bit manipulation logic into common.util
Reused in RssTezUtils and RssMRUtils
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @EnricoMi cc @zhengchenyu

@jerqi jerqi merged commit 1258407 into apache:master Feb 23, 2024
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.

4 participants