lianetm commented on code in PR #20521:
URL: https://github.com/apache/kafka/pull/20521#discussion_r2482077325
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -1901,15 +1897,14 @@ public void
testUpdatePatternSubscriptionEventGeneratedOnlyIfPatternUsed() {
completeUnsubscribeApplicationEventSuccessfully();
consumer.assign(singleton(new TopicPartition("topic1", 0)));
- markReconcileAndAutoCommitCompleteForPollEvent();
+ completeAsyncPollEventSuccessfully();
consumer.poll(Duration.ZERO);
verify(applicationEventHandler,
never()).addAndGet(any(UpdatePatternSubscriptionEvent.class));
consumer.unsubscribe();
consumer.subscribe(Pattern.compile("t*"));
consumer.poll(Duration.ZERO);
-
verify(applicationEventHandler).addAndGet(any(UpdatePatternSubscriptionEvent.class));
Review Comment:
we should completely remove the test in this case (we don't generate this
event on poll anymore)
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -1838,7 +1834,7 @@ void testReaperInvokedInPoll() {
completeTopicSubscriptionChangeEventSuccessfully();
consumer.subscribe(Collections.singletonList("topic"));
when(applicationEventHandler.addAndGet(any(CheckAndUpdatePositionsEvent.class))).thenReturn(true);
Review Comment:
do we still need this? (we don't send this event on poll anymore)
If not needed, let's remove other usages in this test file too.
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumerTest.java:
##########
@@ -1665,11 +1664,8 @@ private void
testUpdateFetchPositionsWithFetchCommittedOffsetsTimeout() {
completeAssignmentChangeEventSuccessfully();
consumer.assign(singleton(new TopicPartition("t1", 1)));
- markReconcileAndAutoCommitCompleteForPollEvent();
+ completeAsyncPollEventSuccessfully();
consumer.poll(Duration.ZERO);
-
- verify(applicationEventHandler, atLeast(1))
-
.addAndGet(ArgumentMatchers.isA(CheckAndUpdatePositionsEvent.class));
Review Comment:
without this verify I don't think the test is checking anything anymore.
Then, looking closer at it, I wonder if this test still makes sense here. It
seemed to ensure that we would fetch partition offsets to reset, or committed
offsets as needed, but that logic does not live in the AsyncConsumer anymore.
So looks to me that
`testRefreshCommittedOffsetsShouldNotResetIfFailedWithTimeout` and
`testRefreshCommittedOffsetsNotCalledIfNoGroupId` dot not apply at the
AsyncConsumer level anymore?
--
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]
