[LiebingYu]

Purpose

Linked issue: close #1389

Brief change log

Tests

API and Format

Documentation

[affo]

Just a doubt about adding a blocking call to ZK

[affo]

Why not getting this out of the existing CoordinatorMetadataCache?
It would avoid a blocking call and fetch directly from memory.

[polyzos]

Thanks @LiebingYu for the work 👍 can you please rebase the branch.. and a few more thoughts:

1. Older servers. getServerNodes() works against any server because it uses the metadata request. With the new RPC, a newer client talking to a 1.0 cluster would fail, for example during a rolling upgrade. Falling back to the current path when the server doesn't support the new RPC, with tags left empty, would keep it working.

2. ZooKeeper read per call. The request goes to a random TabletServer, which doesn't cache tags, so each call reads ZooKeeper on a request thread. The coordinator's metadata cache on main now keeps tags in memory, so serving the call from there would avoid that. Could you reply on @affo's thread with the approach you prefer?

[affo]

@LiebingYu are you still working on this one?
I see your latest commit was 2 months ago here.
I would like to land this one, if it is ok for you I could create a PR cherry-picking your commit and addressing reviewers thoughts so that we can preserve your authorship

[fresh-borzoni]

Found one gap while testing this: if you tag a server and then stop it, list_server_tag doesn't show it anymore, even though the tag is still in ZK. That's pretty much the normal decommission flow (tag -> rebalance -> stop), so the leftover tag just becomes invisible.

I don't think getServerNodes() can fix this nicely, since the coordinator forgets the server's address once it's down. Maybe a small listServerTags() that returns tags by server id, and an alive column in the procedure?

I know #1389 wanted to avoid a separate tags API, but I don't think stopped servers came up there.

The rest seems fine and LGTM in general

[polyzos]

Thank you @fresh-borzoni for catching this.

@LiebingYu I pushed some commits myself to help unblock the work.. I'm merging this for now, @affo do you want to handle the follow-up on what Anton mentioned?

[fresh-borzoni]

@polyzos @LiebingYu @affo
FIP-41 may not need listServerTags() after all. The operator can get what it needs from getServerNodes() (tags on running servers) and describeTabletServers()(stopped servers that hold replicas).

So let's just continue work, and address the gap when it's needed, which I don't see as a blocker for FIP-41.

[swuferhong]

Hi, @polyzos Could we address one availability regression in a follow-up PR：#4873?

getServerNodes() now calls describeCluster through the plain coordinator gateway, bypassing the retry mechanism used by readOnlyGateway. If the cached coordinator address becomes stale after failover, repeated calls can keep targeting the old coordinator even after the cluster has recovered, unless another operation refreshes the metadata.

We can reuse the existing RetryableGatewayClientProxy with a small change:

1. Create a dedicated retryableCoordinatorGateway by wrapping the existing coordinator gateway, reusing metadataUpdater::refreshClusterUntilAvailable and refreshExecutor.
2. Use this proxy for the describeCluster call in getServerNodes(). The existing readOnlyGateway cannot be used directly because it routes requests to a random TabletServer.
3. Keep the UnsupportedVersionException fallback to the metadata request unchanged, preserving compatibility with older servers.
4. Keep write operations on the original plain gateway, since automatically retrying non-idempotent writes could duplicate mutations.

This gives the query the existing recovery behavior: a retriable failure triggers a metadata refresh, followed by one retry against the updated coordinator address.

Please also add a regression test that reuses the same Admin instance, makes the old coordinator unavailable, and has the metadata response advertise a new coordinator. The query should recover automatically and return the expected nodes and tags without requiring an explicit metadata refresh from the caller.

[polyzos]

@swuferhong Thanks for the detailed breakdown 👍

[affo]

FIP-41 may not need listServerTags() after all.

@fresh-borzoni yes yes, that's why I was following this PR closely, as it made the trick without a new endpoint 🤝