EnricoMi commented on code in PR #1434:
URL:
https://github.com/apache/incubator-uniffle/pull/1434#discussion_r1447175607
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -356,7 +353,11 @@ private void waitDecommissionFinish() {
while (isDecommissioning()) {
remainApplicationNum = shuffleTaskManager.getAppIds().size();
if (remainApplicationNum == 0) {
- serverStatus.set(ServerStatus.DECOMMISSIONED);
Review Comment:
State could change between `isDecommissioning()` and
`serverStatus.set(ServerStatus.DECOMMISSIONED)`.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -483,8 +484,11 @@ public NettyMetrics getNettyMetrics() {
}
public boolean isDecommissioning() {
- return ServerStatus.DECOMMISSIONING.equals(serverStatus.get())
- || ServerStatus.DECOMMISSIONED.equals(serverStatus.get());
+ return ServerStatus.DECOMMISSIONING.equals(serverStatus.get());
+ }
Review Comment:
better reflects the name of the method, no need to bind the `while`
condition in `waitDecommissionFinish` on `DECOMMISSIONED` state.
##########
server/src/main/java/org/apache/uniffle/server/HealthCheck.java:
##########
@@ -88,15 +88,13 @@ public HealthCheck(
public void check() {
for (Checker checker : checkers) {
if (!checker.checkIsHealthy()) {
- serverStatus.set(ServerStatus.UNHEALTHY);
+ serverStatus.compareAndSet(ServerStatus.ACTIVE,
ServerStatus.UNHEALTHY);
Review Comment:
The `HealthCheck` should only transition from `ACTIVE` to `UNHEALTHY` and
back. Otherwise, it could stop the decommissioning phase.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -386,21 +387,21 @@ private void waitDecommissionFinish() {
}
public synchronized void cancelDecommission() {
- if (!isDecommissioning()) {
+ boolean wasDecomissioning =
serverStatus.compareAndSet(ServerStatus.DECOMMISSIONING, ServerStatus.ACTIVE);
+ boolean wasDecomissioned =
serverStatus.compareAndSet(ServerStatus.DECOMMISSIONED, ServerStatus.ACTIVE);
+ if (!wasDecomissioning && !wasDecomissioned) {
LOG.info("Shuffle server is not decommissioning. Nothing needs to be
done.");
return;
}
- if (ServerStatus.DECOMMISSIONED.equals(serverStatus.get())) {
- serverStatus.set(ServerStatus.ACTIVE);
- return;
- }
- serverStatus.set(ServerStatus.ACTIVE);
Review Comment:
State could change between `isDecommissioning()` and
`serverStatus.set(ServerStatus.ACTIVE)`.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -328,20 +328,17 @@ public ServerStatus getServerStatus() {
return serverStatus.get();
}
- public void setServerStatus(ServerStatus serverStatus) {
- this.serverStatus.set(serverStatus);
- }
-
public synchronized void decommission() {
- if (isDecommissioning()) {
+ if (isDecommissioning() || isDecommissioned()) {
LOG.info("Shuffle Server is decommissioning. Nothing needs to be done.");
return;
}
- if (!ServerStatus.ACTIVE.equals(serverStatus.get())) {
Review Comment:
State could change between `serverStatus.get()` and
`serverStatus.set(ServerStatus.DECOMMISSIONING)`.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -328,20 +328,17 @@ public ServerStatus getServerStatus() {
return serverStatus.get();
}
- public void setServerStatus(ServerStatus serverStatus) {
Review Comment:
This is likely to introduce race-conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
