AndrewJSchofield commented on code in PR #23651:
URL: https://github.com/apache/kafka/pull/23651#discussion_r4204205715
##########
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##########
@@ -648,7 +648,8 @@ static KafkaAdminClient createInternal(AdminClientConfig
config,
Metrics metrics = null;
String clientId = generateClientId(config);
List<MetricsReporter> reporters =
CommonClientConfigs.metricsReporters(clientId, config);
- Optional<ClientTelemetryReporter> clientTelemetryReporter =
CommonClientConfigs.telemetryReporter(clientId, config);
+ Uuid clientInstanceId = Uuid.randomUuid();
Review Comment:
The KafkaClient will have a client instance ID already but another is being
created here. I wonder if it would be possible to align them. I know this is a
little-used internal constructor for tests, but it's probably worth tightening
up a little so people aren't surprised down the line.
##########
clients/src/main/java/org/apache/kafka/common/requests/PushTelemetryRequest.java:
##########
@@ -45,6 +46,11 @@ public Builder(PushTelemetryRequestData data, boolean
enableUnstableLastVersion)
@Override
public PushTelemetryRequest build(short version) {
+ // From v1 the client instance ID is carried by the request header
(KIP-1313) and the body
+ // field only exists in v0, so a non-zero value must not reach the
writer.
+ if (version >= 1 &&
!Uuid.ZERO_UUID.equals(data.clientInstanceId())) {
+ return new
PushTelemetryRequest(data.duplicate().setClientInstanceId(Uuid.ZERO_UUID),
version);
Review Comment:
Exception for this I think.
##########
clients/src/main/java/org/apache/kafka/common/requests/GetTelemetrySubscriptionsRequest.java:
##########
@@ -40,6 +41,11 @@ public Builder(GetTelemetrySubscriptionsRequestData data,
boolean enableUnstable
@Override
public GetTelemetrySubscriptionsRequest build(short version) {
+ // From v1 the client instance ID is carried by the request header
(KIP-1313) and the body
+ // field only exists in v0, so a non-zero value must not reach the
writer.
+ if (version >= 1 &&
!Uuid.ZERO_UUID.equals(data.clientInstanceId())) {
Review Comment:
Seems like it should never happen. If you look at
`ListConfigResourcesReqeust.java`, you can see that it throws an exception if
the invariant is broken. I think that would be better than silently fixing it
up like this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
