KAFKA-20246: Add clusterId and nodeId to ApiVersionsRequest (3/N) by AndrewJSchofield · Pull Request #22512 · apache/kafka

AndrewJSchofield · 2026-06-08T17:22:49Z

Part of the implementation of
https://cwiki.apache.org/confluence/display/KAFKA/KIP-1242%3A+Detection+and+handling+of+misrouted+connections.

Support coordinator connections and mark ApiVersions v5 API as stable.

Reviewers: TaiJuWu tjwu1217@gmail.com, David Jacot
david.jacot@gmail.com

rajinisivaram

@AndrewJSchofield Thanks for the PR, left a couple of questions.

rajinisivaram · 2026-06-15T09:10:20Z

-                    // To avoid those, only check if the node ID is less than half of Integer.MAX_VALUE.
-                    if (clusterId != null && nodeId >= 0 && nodeId < Integer.MAX_VALUE / 2) {
+                    // We can get the real node ID by subtracting from Integer.MAX_VALUE.
+                    if (nodeId > Integer.MAX_VALUE / 2) {


There is an assumption here that node ids are less than Integer.MAX_VALUE / 2. But that is not a documented limitation for broker ids, right? Isn't it possible that some deployments use larger broker ids, in which case, the larger number is the actual broker id and the smaller one is the coordinator id?

OK, but in that case, wouldn't the entire principle of using large node IDs for the coordinator connections be entirely broken then? We could end up a broker using a very high node ID, and another one which coincidentally has the same node ID when it is being connected as a coordinator. @dajac What do you think? What's a safe path here?

I may be wrong, but I thought for the coordinator, we weren't relying on node ids being low, we were just relying on the node ids being close together, to avoid conflicts between broker node ids and the coordinator ids. Let's see what @dajac says.

Yes, let's. I can think of a way of improving this area if simple arithmetic is not enough.

The coordinator id is just the broker id of the broker hosting the coordinator. Hence, it should usually be low, say < 100000. However, I agree with @rajinisivaram that this is not documented anywhere. Honestly, I don't like the MAX - id hack at all. I wonder if we should rather fix the underlying issue which is that we limit the number of connections by node id. Could you have a way to tell the network client that we want a connection which is not limited by the maximum number of connections allowed? I haven't check how feasible it it though.

My understanding is also that the connection to the group coordinator won't have the nodeId set which is also weird from a design perspective.

Could we override idString in Node to be coordinator-{id} to force the creating of a new connection without hacking the id?

Yes, overriding idString for coordinator connections does work.

rajinisivaram · 2026-06-15T09:10:45Z

  // Version 5 introduces ClusterId and NodeId checking and REBOOTSTRAP_REQUIRED error (KIP-1242).
  "validVersions": "0-5",
  "flexibleVersions": "3+",
-  "latestVersionUnstable": true,


This change is not related to the KIP?

While the implementation was incomplete, I marked the latest version as unstable. This change removes the unstable indication so that new RPC version is now considered stable. That's my intent anyway.

Makes sense.

# Conflicts: # docs/getting-started/upgrade.md

AndrewJSchofield · 2026-06-22T11:00:58Z

The failure of KafkaAdminClientTest.testUnreachableBootstrapNode is not caused by this PR, because there's a latent bug in that test. I'll put in a fix for that too.

dajac · 2026-06-24T13:20:59Z

+        this(id, host, port, rack, isFenced, false);
+    }
+
+    public Node(int id, String host, int port, String rack, boolean isFenced, boolean isCoordinator) {


I wonder if we should generalize it a bit more so we could also use it to have dedicated connection to group coordinator and transaction coordinator in the producer.

Hmm. That's a thought. I will take a look. This is one of those PRs where I'm gradually learning about areas I've not really dug into before.

I've taken a look and this is a bigger piece of work. It's also not necessary in this PR because the way TransactionManager handles the Nodes is much simpler. I propose not to do this enhancement as part of this PR.

dajac · 2026-06-24T13:21:16Z

+            if (id < 0) {
+                throw new IllegalArgumentException("Node id for coordinator node cannot be negative");
+            }
+            this.idString = "+" + id;


why + vs using e.g. coordinator-?

Because I want integer parsing to work. All we need is a difference. There's no need to make this more difficult I feel. I did start with coordinator-[%d} but then I needed a parsing method in Node and I decided the complexity wasn't really worth it.

dajac · 2026-07-01T08:40:27Z

@@ -1138,10 +1138,7 @@ private void handleInitiateApiVersionRequests(long now) {
                if (metadataRecoveryStrategy != MetadataRecoveryStrategy.NONE && metadataClusterCheckEnable) {
                    String clusterId = this.metadataUpdater.clusterId();
                    int nodeId = Integer.parseInt(node);


Can't we just use node.id here?

The Node is generally represented by its ID string in NetworkClient, for example disconnect(String nodeId) and Map<String, ...> nodesNeedingApiVersionsFetch. For the protocol, I want to obtain the numeric ID and I have the ID string in hand, not the actual Node reference.

My bad. I thought that node was a Node :).

dajac

lgtm, thanks

Support coordinator connections and mark API as stable.

5282526

github-actions Bot added core Kafka Broker clients small Small PRs labels Jun 8, 2026

TaiJuWu approved these changes Jun 10, 2026

View reviewed changes

rajinisivaram reviewed Jun 15, 2026

View reviewed changes

AndrewJSchofield added 2 commits June 16, 2026 12:49

Merge branch 'trunk' into KAFKA-20246-4

4aeb76e

# Conflicts: # docs/getting-started/upgrade.md

Remove Integer.MAX_VALUE hack for coordinator node ids

1630745

github-actions Bot added consumer and removed small Small PRs labels Jun 19, 2026

Fix BrokerApiVersionsCommandTest

c9f6894

github-actions Bot added the tools label Jun 22, 2026

Simplify coordinator node string ID

5bbefe9

AndrewJSchofield added 2 commits June 22, 2026 12:17

Fix testUnreachableBootstrapNode

90f17ec

Merge branch 'trunk' into KAFKA-20246-4

0c85129

dajac reviewed Jun 24, 2026

View reviewed changes

dajac reviewed Jul 1, 2026

View reviewed changes

AndrewJSchofield added 3 commits July 2, 2026 10:56

Subclass Node for group coordinator

226c5d4

Spotless

88f5554

Fix BrokerApiVersionsCommandTest

ad59b50

dajac approved these changes Jul 3, 2026

View reviewed changes

Uh oh!

Conversation

AndrewJSchofield commented Jun 8, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield commented Jun 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndrewJSchofield commented Jun 8, 2026 •

edited by github-actions Bot

Loading