-
Notifications
You must be signed in to change notification settings - Fork 148
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
[#80][Part-2] feat: Add RPC logic and heartbeat logic for decommisson #663
Conversation
# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java
# Conflicts: # common/src/main/java/org/apache/uniffle/common/ServerStatus.java # common/src/main/java/org/apache/uniffle/common/exception/InvalidRequestException.java # coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorServer.java # server/src/main/java/org/apache/uniffle/server/ShuffleServer.java # server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java
internal-client/src/main/java/org/apache/uniffle/client/request/RssDecommissionRequest.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #663 +/- ##
============================================
+ Coverage 60.67% 63.02% +2.34%
- Complexity 1802 1836 +34
============================================
Files 216 207 -9
Lines 12458 10677 -1781
Branches 1052 1067 +15
============================================
- Hits 7559 6729 -830
+ Misses 4494 3600 -894
+ Partials 405 348 -57
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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, wait for GA.
common/src/main/java/org/apache/uniffle/common/ServerStatus.java
Outdated
Show resolved
Hide resolved
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java
Outdated
Show resolved
Hide resolved
internal-client/src/main/java/org/apache/uniffle/client/request/RssDecommissionRequest.java
Outdated
Show resolved
Hide resolved
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.
Generally LGTM.
common/src/main/java/org/apache/uniffle/common/ServerStatus.java
Outdated
Show resolved
Hide resolved
Thanks @jerqi @advancedxy |
for (StatusCode statusCode : statusCodes) { | ||
try { | ||
RssProtos.StatusCode.valueOf(statusCode.name()); | ||
} catch (Exception e) { | ||
fail(e.getMessage()); | ||
} | ||
} | ||
for (int i = 0; i < statusCodes.length; i++) { | ||
assertEquals(protoStatusCode[i], statusCodes[i].toProto()); | ||
for (int i = 0; i < statusCodes.size() - 1; i++) { |
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.
this should be restored to the original code since UNKNOWN has already been filtered out.
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.
Get. I will fix it in next pr of this feature.
fail(e.getMessage()); | ||
} | ||
} | ||
for (int i = 0; i < serverStatuses.size() - 1; i++) { |
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.
ditto.. You may open a follow up pr for this.
…misson (apache#663) Add RPC logic and heartbeat logic for decommisson Support shuffle server decommission. It is a part of apache#80 No. UT
…misson (apache#663) ### What changes were proposed in this pull request? Add RPC logic and heartbeat logic for decommisson ### Why are the changes needed? Support shuffle server decommission. It is a part of apache#80 ### Does this PR introduce any user-facing change? No. ### How was this patch tested? UT
What changes were proposed in this pull request?
Add RPC logic and heartbeat logic for decommisson
Why are the changes needed?
Support shuffle server decommission. It is a part of #80
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UT