You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "kirktrue (via GitHub)" <gi...@apache.org> on 2025/10/02 17:36:47 UTC
[PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
kirktrue opened a new pull request, #20631:
URL: https://github.com/apache/kafka/pull/20631
This change introduces `AbstractConsumerMetricsManager` as a shared base class for consumer metrics managers, consolidating the steps of registering and cleanup logic.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
Posted by "kirktrue (via GitHub)" <gi...@apache.org>.
kirktrue commented on code in PR #20631:
URL: https://github.com/apache/kafka/pull/20631#discussion_r2535966461
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -416,7 +418,7 @@ public AsyncKafkaConsumer(final ConsumerConfig config,
final List<InetSocketAddress> addresses = ClientUtils.parseAndValidateAddresses(config);
metadata.bootstrap(addresses);
- FetchMetricsManager fetchMetricsManager = createFetchMetricsManager(metrics);
+ this.fetchMetricsManager = createFetchMetricsManager(metrics);
Review Comment:
Done! PTAL. Thanks!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
Posted by "kirktrue (via GitHub)" <gi...@apache.org>.
kirktrue commented on code in PR #20631:
URL: https://github.com/apache/kafka/pull/20631#discussion_r2458209358
##########
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java:
##########
@@ -389,6 +389,19 @@ public void testMetricsReporterAutoGeneratedClientId(GroupProtocol groupProtocol
consumer.close(CloseOptions.timeout(Duration.ZERO));
}
+ @ParameterizedTest
+ @EnumSource(GroupProtocol.class)
+ public void testMetricsRemovedOnClose(GroupProtocol groupProtocol) {
+ Properties props = new Properties();
+ props.setProperty(ConsumerConfig.GROUP_PROTOCOL_CONFIG, groupProtocol.name());
+ props.setProperty(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999");
+ consumer = newConsumer(props, new StringDeserializer(), new StringDeserializer());
+
+ assertTrue(consumer.metrics().size() > 1, "The consumer should have created many metrics");
+ consumer.close(CloseOptions.timeout(Duration.ZERO));
+ assertTrue(consumer.metrics().size() <= 1, "The consumer should have removed all of its metrics");
Review Comment:
Thanks for the review @LiamClarkeNZ.
Yes, there's this meta metric named `count` that is present by default. I've updated the test to hopefully be more clear about that.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
Posted by "LiamClarkeNZ (via GitHub)" <gi...@apache.org>.
LiamClarkeNZ commented on code in PR #20631:
URL: https://github.com/apache/kafka/pull/20631#discussion_r2403769686
##########
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java:
##########
@@ -389,6 +389,19 @@ public void testMetricsReporterAutoGeneratedClientId(GroupProtocol groupProtocol
consumer.close(CloseOptions.timeout(Duration.ZERO));
}
+ @ParameterizedTest
+ @EnumSource(GroupProtocol.class)
+ public void testMetricsRemovedOnClose(GroupProtocol groupProtocol) {
+ Properties props = new Properties();
+ props.setProperty(ConsumerConfig.GROUP_PROTOCOL_CONFIG, groupProtocol.name());
+ props.setProperty(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999");
+ consumer = newConsumer(props, new StringDeserializer(), new StringDeserializer());
+
+ assertTrue(consumer.metrics().size() > 1, "The consumer should have created many metrics");
+ consumer.close(CloseOptions.timeout(Duration.ZERO));
+ assertTrue(consumer.metrics().size() <= 1, "The consumer should have removed all of its metrics");
Review Comment:
Curious about the `> 1` and `<= 1`, reads like there's usually a metric just hanging around in there?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
Posted by "ShivsundarR (via GitHub)" <gi...@apache.org>.
ShivsundarR commented on code in PR #20631:
URL: https://github.com/apache/kafka/pull/20631#discussion_r2533424516
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -416,7 +418,7 @@ public AsyncKafkaConsumer(final ConsumerConfig config,
final List<InetSocketAddress> addresses = ClientUtils.parseAndValidateAddresses(config);
metadata.bootstrap(addresses);
- FetchMetricsManager fetchMetricsManager = createFetchMetricsManager(metrics);
+ this.fetchMetricsManager = createFetchMetricsManager(metrics);
Review Comment:
Should we do a similar change to `ShareConsumerImpl` to have `shareFetchMetricsManager` as a field and close it during close().
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
Posted by "ShivsundarR (via GitHub)" <gi...@apache.org>.
ShivsundarR commented on PR #20631:
URL: https://github.com/apache/kafka/pull/20631#issuecomment-3540934695
Thanks for the PR @kirktrue , overall looks good to me, just had one question.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics [kafka]
Posted by "kirktrue (via GitHub)" <gi...@apache.org>.
kirktrue closed pull request #20631: KAFKA-19542: Consumer.close() does not remove all added sensors from Metrics
URL: https://github.com/apache/kafka/pull/20631
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org