[#956] refactor: Changes the Boolean flag that determines whether a Node is healthy to a state #959
Conversation
proto/src/main/proto/Rss.proto
Outdated
| @@ -251,7 +251,6 @@ message ShuffleServerHeartBeatRequest { | |||
| int64 availableMemory = 4; | |||
| int32 eventNumInFlush = 5; | |||
| repeated string tags = 6; | |||
| google.protobuf.BoolValue isHealthy = 7; | |||
There was a problem hiding this comment.
Don't modify the field. You can add a comment // deprecated. If you modify this, this will be incompatible feature. Although we don't release 1.0 version, we don't need to guarantee the compatibility, I still hope we reduce the similar breaking change.
jerqi
changed the title
| ShuffleServerMetrics.gaugeIsHealthy.set(1); | ||
| return; | ||
| } | ||
| } | ||
| ShuffleServerMetrics.gaugeIsHealthy.set(0); | ||
| isHealthy.set(true); | ||
| if (shuffleServer.getServerStatus() == ServerStatus.DECOMMISSIONING) { |
There was a problem hiding this comment.
In ShuffleServer. After decommission is called, there are a large number of application is not complete case, ServerStatus state is DECOMMISSIONING, but we can't change the state of ServerStatus health thread detection
There was a problem hiding this comment.
why do we assign DECOMMISSIONING to it if its origin value is DECOMMISSIONING? It seems that we don't change value.
There was a problem hiding this comment.
sorry, I made a mistake
| @@ -226,7 +226,7 @@ private void initialization() throws Exception { | |||
| if (healthCheckEnable) { | |||
| List<Checker> builtInCheckers = Lists.newArrayList(); | |||
| builtInCheckers.add(storageManager.getStorageChecker()); | |||
| healthCheck = new HealthCheck(isHealthy, shuffleServerConf, builtInCheckers); | |||
| healthCheck = new HealthCheck(this, shuffleServerConf, builtInCheckers); | |||
There was a problem hiding this comment.
You can pass ServerStatus as parameter. We can use AtomicReference as the type of ServerStatus.
| private final long checkIntervalMs; | ||
| private final Thread thread; | ||
| private volatile boolean isStop = false; | ||
| private List<Checker> checkers = Lists.newArrayList(); | ||
|
|
||
| public HealthCheck(AtomicBoolean isHealthy, ShuffleServerConf conf, List<Checker> buildInCheckers) { | ||
| this.isHealthy = isHealthy; | ||
| public HealthCheck(AtomicReference<ServerStatus> serverStatus, |
There was a problem hiding this comment.
Could we use style consistent with other places?
public HealthCheck(
AtomicReference<ServerStatus> serverStatus,
ShuffleServerConf conf,
List<Checker> buildInCheckers) {
There was a problem hiding this comment.
@yl09099 Could you address this comment?
There was a problem hiding this comment.
sorry, Have been changed
| boolean isHealthy = true; | ||
| if (request.hasIsHealthy()) { | ||
| isHealthy = request.getIsHealthy().getValue(); | ||
| /** | ||
| * Compatible with isHealthy version |
There was a problem hiding this comment.
| * Compatible with isHealthy version | |
| * Compatible with older version |
| @@ -1,38 +1,26 @@ | |||
| /* | |||
There was a problem hiding this comment.
Why do we remove the license header?
There was a problem hiding this comment.
I moved this file, forgot to add it, let me add it
…ether a Node is healthy to a state
What changes were proposed in this pull request?
Change the ServerNode health status from the original Boolean judgment to the unhealthy state
Why are the changes needed?
Unhealthy states should not be isolated
Fix: #956
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing unit tests