-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MINOR] Migrate RankValue to the package of the common class #265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #265 +/- ##
============================================
+ Coverage 59.70% 61.10% +1.40%
- Complexity 1377 1516 +139
============================================
Files 166 186 +20
Lines 8916 9408 +492
Branches 853 918 +65
============================================
+ Hits 5323 5749 +426
- Misses 3318 3355 +37
- Partials 275 304 +29
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Please cc, just a minor change. @jerqi |
Could you give me a concrete example? Why do we need this pr? |
Because this static class is currently in |
Why do we migrate RankValue to the package of the common class if shuffle server and client won't use RankValue. |
I see what you mean, but I think it is necessary to set it as a separate class. Put it under the coordinator package ? |
Coordinator package is better. |
@smallzhongfeng Will you continue this pr? |
Done, sorry to take so long. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @smallzhongfeng
What changes were proposed in this pull request?
More standardized, and the class can be extended in the future.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
No need.