Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-18600 Cleanup NetworkClient zk related logging #18644

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
+2 −2
Open

KAFKA-18600 Cleanup NetworkClient zk related logging #18644

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6806792
update the log
m1a2st Jan 20, 2025
38bf18d
Merge branch 'trunk' into KAFKA-18600
m1a2st Jan 30, 2025
d8ea2df
Merge branch 'trunk' into KAFKA-18600
m1a2st Feb 18, 2025
718031d
Merge branch 'trunk' into KAFKA-18600
m1a2st Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions clients/src/main/java/org/apache/kafka/clients/NetworkClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1051,9 +1051,9 @@ private void handleApiVersionsResponse(List<ClientResponse> responses,
apiVersionsResponse.data().finalizedFeaturesEpoch());
apiVersions.update(node, nodeVersionInfo);
this.connectionStates.ready(node);
log.debug("Node {} has finalized features epoch: {}, finalized features: {}, supported features: {}, ZK migration ready: {}, API versions: {}.",
log.debug("Node {} has finalized features epoch: {}, finalized features: {}, supported features: {}, API versions: {}.",
node, apiVersionsResponse.data().finalizedFeaturesEpoch(), apiVersionsResponse.data().finalizedFeatures(),
apiVersionsResponse.data().supportedFeatures(), apiVersionsResponse.data().zkMigrationReady(), nodeVersionInfo);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Unrelated to this PR) @mumrah Do we have a plan to remove this field from the protocol api? We would need to ensure it's done in a compatible way, but it would be nice not to have this field in request logs and general logging.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a tagged field and 4.0 cannot be the target of a ZK migration, we could just remove it without a version bump. However, we might consider keeping it around so that 4.0+ controllers can reject errant registrations from a Zk broker (like due to a misconfiguration).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, would this be from a broker that is being migrated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually I was confusing this with the same field in the broker registration request. Looking at usages in 3.9, I'm not sure we ever used this field. I'll dig in a bit on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field (ZkMigrationReady in ApiVersionsResponse) was last used in 3.6. Starting in 3.7, we used the controller registration RPC in lieu of this field. So, it seems a bit like dead code at this point. We should probably leave the field in the RPC so we don't reuse the tagged field number, but we can set the max versions to v4 and remove the usages in the non-generated code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can set the max versions to v4

Pardon me, it seems to me the max version is already set to v4 (ApiKeys.API_VERSIONS.latestVersion()). Please correct me if I misunderstand anything.

we might consider keeping it around so that 4.0+ controllers can reject errant registrations from a Zk broker (like due to a misconfiguration).

it seems that is addressed already.

kafka/metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java

Line 362 in be7415c

throw new BrokerIdNotRegisteredException("Controller does not support registering ZK brokers.");

remove the usages in the non-generated code.

I prefer to remove isMigratingZkBroker and IsMigratingZkBroker from non-generated code. similar to my previous comment #17293 (comment)

apiVersionsResponse.data().supportedFeatures(), nodeVersionInfo);
}

/**
Expand Down