lianetm commented on code in PR #17150:
URL: https://github.com/apache/kafka/pull/17150#discussion_r1825881712
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -863,7 +861,8 @@ public void testAutoCommitSyncDisabled() {
mock(ConsumerRebalanceListenerInvoker.class),
subscriptions,
"group-id",
- "client-id");
+ "client-id",
+ false);
Review Comment:
not introduced by this PR, but this change made me notice that this test is
not really testing what it should. We're verifying that no commit sync event is
generated if autoCommit false, but we're only doing consumer.subscribe & seek
(which will never generate a sync event anyways). I expect this test would also
pass if we pass autoCommit=true.
This test should explicitly call consumer.close after the seek, and then
verify that no SyncEvent is generated if autoCommit=false, makes sense?
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -844,14 +837,19 @@ public void testAutoCommitSyncEnabled() {
mock(ConsumerRebalanceListenerInvoker.class),
subscriptions,
"group-id",
- "client-id");
+ "client-id",
+ false);
Review Comment:
this `false` is confusing given that the test is called
"testAutoCommitSyncEnabled", but I notice it's because the test does not take
the config into account and calls the `commitSyncAllConsumed` explicitly, so
what about renaming it to `testCommitSyncAllConsumed`?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
