[server] Add Cluster Health API implementation#3400
Conversation
swuferhong
force-pushed
the
fluss-server-recovery
branch
from
fe0c5c1 to
042fc7a
Compare
loserwang1024
left a comment
loserwang1024
left a comment
There was a problem hiding this comment.
I have left some comment.
| * send NotifyLeaderAndIsr until the target server responds successfully confirming it is the | ||
| * leader. Also inactive when leader == NO_LEADER. | ||
| */ | ||
| private final Set<TableBucket> inactiveLeaderBuckets = new HashSet<>(); |
There was a problem hiding this comment.
Naming: inactiveLeaderBuckets → pendingLeaderActivationBuckets
The name over-claims. The set is only populated in sendNotifyLeaderAndIsrRequest — buckets where we've sent a NotifyLeaderAndIsr and await leadership confirmation. Buckets leaderless for other reasons (server offline, rebalance with no new leader yet) aren't in it, though their leader is effectively inactive. So it really means "leader change dispatched, awaiting confirmation" — pendingLeaderActivationBuckets fits. Avoid pendingIsr...: this has nothing to do with ISR (that's a separate dimension driving YELLOW).
Bigger issue: "active leader" is defined twice. computeClusterHealth checks leader != NO_LEADER && liveServers.contains(leader) && !inSet, but the helper only checks the set:
public boolean isLeaderActive(TableBucket bucket) {
return !inactiveLeaderBuckets.contains(bucket); // ignores liveServers / NO_LEADER
}So for a leader on a dead server, isLeaderActive() says true while computeClusterHealth() says RED — inconsistent, and a trap for future reuse.
Suggestion: rename the set + make isLeaderActive the single source of truth:
public boolean isLeaderActive(TableBucket tb) {
return getBucketLeaderAndIsr(tb)
.map(lai -> lai.leader() != LeaderAndIsr.NO_LEADER
&& liveTabletServers.containsKey(lai.leader())
&& !pendingLeaderActivationBuckets.contains(tb))
.orElse(false);
}Then computeClusterHealth just calls ctx.isLeaderActive(tb).
|
|
||
| // PbClusterHealthStatus: GREEN=0, YELLOW=1, RED=2, UNKNOWN=3 | ||
| int status; | ||
| if (numLeaderReplicas == 0) { |
There was a problem hiding this comment.
This can merge with else {
status = 0; // GREEN
}
| } | ||
|
|
||
| @Override | ||
| public CompletableFuture<GetClusterHealthResponse> getClusterHealth( |
There was a problem hiding this comment.
Why not move to CoordinatorGateway? Could tablet server support it?
| } | ||
| } | ||
| if (!offlineReplicas.isEmpty()) { | ||
| // trigger replicas to offline |
There was a problem hiding this comment.
why remove this comment?
| // re-election via onReplicaBecomeOffline. | ||
| List<NotifyLeaderAndIsrResultForBucket> failedResults = | ||
| new ArrayList<>(); | ||
| ApiError sendError = |
There was a problem hiding this comment.
Currently, after this modification, even a network timeout exception, the CoordinatorEventProcessor#processNotifyLeaderAndIsrResponseReceivedEvent will mark the server as offlineReplicas
won't is any problem?
| echo "advertised.listeners: ${ADVERTISED_LISTENERS}" >> $FLUSS_HOME/conf/server.yaml && \ | ||
|
|
||
| bin/coordinator-server.sh start-foreground | ||
| exec bin/coordinator-server.sh start-foreground |
There was a problem hiding this comment.
why need to change it?
2 participants
Purpose
Linked issue: close #3399
GetClusterHealthRPC to Coordinator that computes cluster health from in-memory stateCoordinatorContext(marked inactive onNotifyLeaderAndIsrsend,marked active on successful response when responding server is still the leader)
CoordinatorRequestBatchby synthesizing error responses to clearpending inactive state
Admin.getClusterHealth()withClusterHealth/ClusterHealthStatustypesClusterHealthReadinessCheckCLI tool in fluss-dist (exit 0=GREEN, 1=not ready, 2=API unsupported)readiness-check.shtwo-step readiness probe script (TCP + Cluster Health API)with first-boot detection and grace period for API-unsupported (mixed-version rolling upgrade)
readiness-check.shin Helm chartBrief change log
Tests
API and Format
Documentation