Repository navigation
Conversation
b3d64f5 to
2aad05f
Compare
affo
left a comment
There was a problem hiding this comment.
Matches the expectations for unlocking a detailed description of the tablet server and assessing a server health on a per-server basis.
Thanks for this contribution!
|
@affo good idea! So you think it makes sense as follow up PR or fine to add here? |
|
@morazow I would directly include that in this PR, as semantically it would make sense as we "describe" tablet servers. But I would also like to have an additional feedback on this one as well 🤝 |
fresh-borzoni
left a comment
There was a problem hiding this comment.
@morazow Thank you for the PR!
Reading this alongside #1410 and #4061, the split looks like:
#1410 DescribeCluster for topology + ServerTag, getClusterHealth for aggregates, this one for per-server counters
#4061 the cache both health APIs read from.
If it holds, two things follow here: the tag commit is redundant with #1410, and both PRs currently claim ApiKey 1065 so one needs renumbering.
I'd hold the API-shape questions until that settles rather than have you redo work.
let me know when it settles, so I can help with API-shape
| assertThat(isServerGreen(server)).isTrue(); | ||
| } | ||
|
|
||
| // The per-server counters must sum up to the cluster-wide health counters. |
There was a problem hiding this comment.
A leaderless bucket is counted by cluster health but by no server here, so these sums can differ while every server still looks green. The waitUntil above doesn't rule that out. testPerServerSumsReconcileWithClusterHealth already covers this.
Do we need it?
|
@fresh-borzoni at issue #1389 we conducted quite a big discussion also on the performance of this call given that it runs in the coordinator loop itself. This led me to shape in draft a PR for an health cache that can be used by both cluster health and per-tablet health: #4061. The cache uses a new mechanism thought for the performance updates, a new CoalescingRefreshCache class that embeds the logic for no-read-lock, but write lock, and enhances the logic to allow for different dirty states to optimize heavy updates on the coordinator side. The same mechanism can be re-used across caches as well. What would you guys think (@fresh-borzoni , @morazow )? In case that one seems like a good approach we can:
I re-read your comment above and realized you are aware of #4061 🤝 😅 , I misread at the beginning. |
|
Thanks @affo , @fresh-borzoni for review! It would make sense to wait for #4061 and rebase to use the new cache mechanism. I'll also drop the serverTags commit from this PR 🤝 |
482d2ba to
128cc24
Compare
|
@morazow as per #4061 (comment) this one is unblocked and does not need to wait 🤝 |
|
yeap, going to rebase and fix conflicts 🤝 |
4ff2e5f to
c167c2c
Compare
| } | ||
|
|
||
| // tagged servers are being drained, so report them even when dead and empty | ||
| ctx.getServerTags() |
There was a problem hiding this comment.
| assertThat(isServerGreen(server)).isTrue(); | ||
| } | ||
|
|
||
| // The per-server counters must sum up to the cluster-wide health counters. |
There was a problem hiding this comment.
Following up on @fresh-borzoni's earlier comment, which is now outdated after the test moved: a bucket without a leader counts in cluster health but on no server, so these totals can differ even while every server looks healthy. The wait above doesn't rule that out, so this check could be flaky. The reconciliation unit test already covers this reliably, so dropping this block seems safest.
| * numReplicas && activeLeaderReplicas == numLeaderReplicas}. | ||
| * </ul> | ||
| * | ||
| * <p>Note: a bucket without an elected leader is attributed to no server's {@code |
There was a problem hiding this comment.
This note explains why the per-server check isn't enough on its own: a bucket without a leader can leave every server that hosts it looking healthy. So the operator also has to ask for cluster health separately, and the two answers can come from different moments. As @fresh-borzoni suggested, computing both in one pass, or adding a per-server count of buckets without a leader, would let the operator decide from a single response. Since this is new public API, it's easiest to settle these things first.
|
Thanks for the rebase @morazow, I left a few inline notes on the points from the earlier discussion that are still open: the server tags, a single consistent answer for the upgrade check, and the totals check in the rolling-upgrade test. The CI failures are unrelated here, so we can ignore them. |
…upgrades Add a read-only DescribeTabletServers RPC to the Coordinator, following the pattern of GetClusterHealth (apache#3399). It returns, per tablet server, the same four counters getClusterHealth() reports cluster-wide, scoped to that server: numReplicas, inSyncReplicas, numLeaderReplicas and activeLeaderReplicas. It also returns leaderlessReplicas: the number of replicas on that server whose bucket has no elected leader. This lets operational tooling (e.g. the Fluss Kubernetes Operator): - gate scale-in on a server being empty (numReplicas == 0), - gate rolling upgrades on a per-server green predicate (inSyncReplicas == numReplicas && activeLeaderReplicas == numLeaderReplicas && leaderlessReplicas == 0), - report per-server tablet load in cluster status. Every reported server is green exactly when getClusterHealth() reports GREEN, so a single response is enough for a rolling-upgrade gate. The counters are computed from in-memory CoordinatorContext state on the coordinator event thread via AccessContextEvent. Tablet servers forward the request to the coordinator. Live and shutting-down servers are always reported, even with zero replicas, and dead servers that still hold assigned replicas remain visible so a scale-in gate cannot treat them as drained. Closes apache#3570
c167c2c to
ed8eb42
Compare
|
Hello @polyzos, @fresh-borzoni thanks for the detailed review! I have addressed your findings, please have another look 🤝 |
Summary
DescribeTabletServersRPC to the Coordinator, following the pattern ofGetClusterHealth([server] Support Cluster Health API for safe rolling upgrades #3399 / [server] Add Cluster Health API implementation #3400).Admin.describeTabletServers()(@PublicEvolving) returns, per tablet server, the same four countersgetClusterHealth()reports cluster-wide, scoped to that server:numReplicas,inSyncReplicas,numLeaderReplicas,activeLeaderReplicas.CoordinatorContextstate on the coordinator event thread (viaAccessContextEvent); tablet servers forward the request to the coordinator over the internal listener.serverGreen(serverId)rolling-upgrade gate, the scale-in safety gate (numReplicas == 0), and per-server tablet load reporting.Fixes #3570
Test Plan
DescribeTabletServersTest(fluss-server) covering: empty cluster, full ISR, out-of-ISR replicas, inactive leaders, leaderless buckets, evacuated servers, dead-but-assigned servers, partitioned tables, and reconciliation of per-server sums withcomputeClusterHealth.FlussAdminITCase#testDescribeTabletServersDuringRollingUpgrade: healthy cluster is green and sums match cluster health; a stopped server stays reported, non-empty and non-green; after restart all servers become green again.mvn spotless:checkclean on fluss-rpc, fluss-server, fluss-client, fluss-flink-common.🤖 AI-assisted changes - reviewed by human developer