[#1433] fix(server): Race conditions with ShuffleServer state - #1434
Conversation
Avoids the following race conditions / undesired state changes: - ShuffleServer is decommissioning, then getting unhealthy, which stops decommissioning - ShuffleServer is in any non-active state, getting unhealthy, turns active when healthy
| return serverStatus.get(); | ||
| } | ||
|
|
||
| public void setServerStatus(ServerStatus serverStatus) { |
There was a problem hiding this comment.
This is likely to introduce race-conditions.
| LOG.info("Shuffle Server is decommissioning. Nothing needs to be done."); | ||
| return; | ||
| } | ||
| if (!ServerStatus.ACTIVE.equals(serverStatus.get())) { |
There was a problem hiding this comment.
State could change between serverStatus.get() and serverStatus.set(ServerStatus.DECOMMISSIONING).
| while (isDecommissioning()) { | ||
| remainApplicationNum = shuffleTaskManager.getAppIds().size(); | ||
| if (remainApplicationNum == 0) { | ||
| serverStatus.set(ServerStatus.DECOMMISSIONED); |
There was a problem hiding this comment.
State could change between isDecommissioning() and serverStatus.set(ServerStatus.DECOMMISSIONED).
| serverStatus.set(ServerStatus.ACTIVE); | ||
| return; | ||
| } | ||
| serverStatus.set(ServerStatus.ACTIVE); |
There was a problem hiding this comment.
State could change between isDecommissioning() and serverStatus.set(ServerStatus.ACTIVE).
| public boolean isDecommissioning() { | ||
| return ServerStatus.DECOMMISSIONING.equals(serverStatus.get()) | ||
| || ServerStatus.DECOMMISSIONED.equals(serverStatus.get()); | ||
| return ServerStatus.DECOMMISSIONING.equals(serverStatus.get()); | ||
| } |
There was a problem hiding this comment.
better reflects the name of the method, no need to bind the while condition in waitDecommissionFinish on DECOMMISSIONED state.
| for (Checker checker : checkers) { | ||
| if (!checker.checkIsHealthy()) { | ||
| serverStatus.set(ServerStatus.UNHEALTHY); | ||
| serverStatus.compareAndSet(ServerStatus.ACTIVE, ServerStatus.UNHEALTHY); |
There was a problem hiding this comment.
The HealthCheck should only transition from ACTIVE to UNHEALTHY and back. Otherwise, it could stop the decommissioning phase.
EnricoMi
changed the title
| boolean wasDecommissioning = | ||
| serverStatus.compareAndSet(ServerStatus.DECOMMISSIONING, ServerStatus.DECOMMISSIONED); | ||
| if (!wasDecommissioning) { | ||
| break; |
There was a problem hiding this comment.
I think we should print some logs here.
There was a problem hiding this comment.
We are leaving the lop here and there will be a log when exiting this method:
LOG.info("Decommission exiting, remaining {} applications not finished.", remainApplicationNum);
There was a problem hiding this comment.
remainApplicationNum is 0, so this line will not be reached. right?
There was a problem hiding this comment.
What about
| break; | |
| LOG.info("Ready to decommission but decommissioning state left unexpectedly."); | |
| break; |
There was a problem hiding this comment.
What about
It is ok for me.
There was a problem hiding this comment.
You are right, we won't see that existing log message.
I have moved the "All applications finished. Current status is " + serverStatus message up so it is printed in any case logging the current stats, and added the suggested additional log line.
|
I presume Java 8 support is still a requirement? |
Sorry. I don't get your point. |
In some places it would be preferably to use |
Get it. We still need to support Java 8. Please see #674 |
2 participants
What changes were proposed in this pull request?
Make state transitions in ShuffleServer robust.
Why are the changes needed?
Avoids the following race conditions / undesired state transitions:
Fix: #1433
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Untested.