lianetm commented on code in PR #20491:
URL: https://github.com/apache/kafka/pull/20491#discussion_r2325603602
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -2029,6 +2030,27 @@ public void
testRecordBackgroundEventQueueSizeAndBackgroundEventQueueTime() {
assertEquals(10, (double)
metrics.metric(metrics.metricName("background-event-queue-time-max",
CONSUMER_METRIC_GROUP)).metricValue());
}
+ @Test
+ public void testFailConstructor() {
+ final Properties props = requiredConsumerConfig();
+ props.put(ConsumerConfig.GROUP_ID_CONFIG, "group-id");
+ props.put(ConsumerConfig.METRIC_REPORTER_CLASSES_CONFIG,
"an.invalid.class");
+ final ConsumerConfig config = new ConsumerConfig(props);
+
+ LogCaptureAppender appender = LogCaptureAppender.createAndRegister();
Review Comment:
should we put this in a try-with-resources to ensure it's properly closed?
(not sure about the internals of LogCaptureAppenders and how it works across
multiple tests, but we've seen already several flaky tests related to it, so
better to clean things up)
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -2029,6 +2030,27 @@ public void
testRecordBackgroundEventQueueSizeAndBackgroundEventQueueTime() {
assertEquals(10, (double)
metrics.metric(metrics.metricName("background-event-queue-time-max",
CONSUMER_METRIC_GROUP)).metricValue());
}
+ @Test
+ public void testFailConstructor() {
+ final Properties props = requiredConsumerConfig();
+ props.put(ConsumerConfig.GROUP_ID_CONFIG, "group-id");
Review Comment:
Interesting, trying to understand why we needed this, I realized that it was
probably to make sure there is a non-null `groupMetadata`, so you can ensure
that the test pass because it hits they newly added null check on
`appEventHandler`, and not because of the existing early return on
groupMetadata.isEmpty, correct? But then I realized that the `groupMetadata`
creation happens after the metrics anyways, so it will end up always being null
for this test anyways, right? (meaning I expect this test would pass, no noisy
NPE even without this PR)
That being said, I think the sanity checks make sense, and the test will
surely help catch any regression (a simple change or order in the actions of
the constructor would make the noisy NPEs appear I expect).
So no changes I would say, just thinking out loud here and sharing for the
record.
--
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]
