KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests #20765

brandboat · 2025-10-24T15:42:03Z

This patch wraps ShareFetchUtils static method processFetchResponse
with MockedStatic to improve test isolation and also fixes some
incorrect test results.

Reviewers: Abhinav Dixit [email protected], Andrew Schofield
[email protected]

brandboat · 2025-10-24T16:08:59Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

+            assertNull(shareGroupMetrics.topicPartitionsFetchRatio(groupId));
+
+            delayedShareFetch.lock().unlock();
+            Mockito.verify(exceptionHandler, never()).accept(any(), any());


exceptionHandler shouldn't be triggered in this test case.
The reason it throw error simply due to incorrect test mock up,

kafka/core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

Line 238 in 342a8e6

when(replicaManager.getPartitionOrException(tp0.topicPartition())).thenReturn(p0);

the above code snippet override the return value of replicaManager.getPartitionOrException(tp0.topicPartition()) defined in

kafka/core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

Line 225 in 342a8e6

mockTopicIdPartitionFetchBytes(replicaManager, tp0, hwmOffsetMetadata);

, which make isMinBytesSatisfied raise NPE

kafka/core/src/main/java/kafka/server/share/DelayedShareFetch.java

Line 481 in 43a2504

exceptionHandler.accept(

, and this is unexpected.

brandboat · 2025-10-24T16:09:41Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

        assertTrue(delayedShareFetch.lock().tryLock());
        delayedShareFetch.lock().unlock();
-        Mockito.verify(exceptionHandler, times(1)).accept(any(), any());
+        Mockito.verify(exceptionHandler, never()).accept(any(), any());


brandboat · 2025-10-25T03:07:35Z

UserQuotaTest#testThrottledRequest(String) with groupProtocol=classic is a known flaky test (KAFKA-8459). It’s unrelated to this PR and passes locally.

brandboat · 2025-10-25T03:15:40Z

Gentle ping , @AndrewJSchofield, @apoorvmittal10, @adixitconfluent — sorry for the late implementation 🙏
I’ve also fixed a few incorrect tests in this patch. Would appreciate your review when you have a chance. Thanks!

apoorvmittal10 · 2025-10-26T14:42:49Z

@adixitconfluent Can you please review the PR.

adixitconfluent

Thanks for the PR @brandboat . I've left some comments.

adixitconfluent · 2025-10-27T08:45:31Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java


        // We are testing the case when the share partition is getting fetched for the first time, so for the first time
-        // the fetchOffsetMetadata will return empty. Post the readFromLog call, the fetchOffsetMetadata will be
+        // the fetchOffsetMetadata will return empty. Post the isMinBytesSatisfied call, the fetchOffsetMetadata will be


I think the comment that makes more sense is Post the first readFromLog call...

Oh, you’re right. I misread the ...populated for the share partition,... part. I initially thought the comment was explaining where we trigger fetchOffsetMetadata. Thanks for the correction!

adixitconfluent · 2025-10-27T08:45:36Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

-        doAnswer(invocation -> buildLogReadResult(List.of(tp0))).when(replicaManager).readFromLog(any(), any(), any(ReplicaQuota.class), anyBoolean());
-        BiConsumer<SharePartitionKey, Throwable> exceptionHandler = mockExceptionHandler();
-
-        PartitionMaxBytesStrategy partitionMaxBytesStrategy = mockPartitionMaxBytes(Set.of(tp0));


Any reason why this piece of code has been moved below - basically a cut and paste?

yes, I moved the code snippet from mockTopicIdPartitionFetchBytes to here and did some refactoring.

adixitconfluent · 2025-10-27T08:45:44Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

-    private void mockTopicIdPartitionFetchBytes(ReplicaManager replicaManager, TopicIdPartition topicIdPartition, LogOffsetMetadata hwmOffsetMetadata) {
-        LogOffsetSnapshot endOffsetSnapshot = new LogOffsetSnapshot(1, mock(LogOffsetMetadata.class),
-            hwmOffsetMetadata, mock(LogOffsetMetadata.class));
-        Partition partition = mock(Partition.class);
-        when(partition.fetchOffsetSnapshot(any(), anyBoolean())).thenReturn(endOffsetSnapshot);
-        when(replicaManager.getPartitionOrException(topicIdPartition.topicPartition())).thenReturn(partition);
-    }
-


Removal of this function doesn't make sense to me. I agree that the line when(replicaManager.getPartitionOrException(topicIdPartition.topicPartition())).thenReturn(partition); is problematic. We can pass the mock Partition object into this function. Example -

caller function -

Partition p0 = mock(Partition.class); mockTopicIdPartitionFetchBytes(replicaManager, tp0, hwmOffsetMetadata, p0); when(p0.isLeader()).thenReturn(true); when(replicaManager.getPartitionOrException(tp0.topicPartition())).thenReturn(p0);

function mockTopicIdPartitionFetchBytes -

private void mockTopicIdPartitionFetchBytes(LogOffsetMetadata hwmOffsetMetadata, Partition partition) { LogOffsetSnapshot endOffsetSnapshot = new LogOffsetSnapshot(1, mock(LogOffsetMetadata.class), hwmOffsetMetadata, mock(LogOffsetMetadata.class)); when(partition.fetchOffsetSnapshot(any(), anyBoolean())).thenReturn(endOffsetSnapshot); }

Sure, I will add back the method.

adixitconfluent · 2025-10-27T09:02:32Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java


-        assertFalse(delayedShareFetch.isCompleted());
+        try (MockedStatic<ShareFetchUtils> mockedShareFetchUtils = Mockito.mockStatic(ShareFetchUtils.class, Mockito.CALLS_REAL_METHODS)) {
+            mockedShareFetchUtils.when(() -> ShareFetchUtils.processFetchResponse(any(), any(), any(), any(), any()))


I don't think we need mocking of processFetchResponse since it is not being called in this test

I agree with you, remove

when(sp0.acquire(any(), anyInt(), anyInt(), anyLong(), any(), any())).thenReturn( createShareAcquiredRecords(new ShareFetchResponseData.AcquiredRecords().setFirstOffset(0).setLastOffset(3).setDeliveryCount((short) 1)));

in L215~215 should be enough.

adixitconfluent · 2025-10-27T11:06:53Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

        when(replicaManager.getPartitionOrException(tp0.topicPartition())).thenReturn(p0);
        when(replicaManager.getPartitionOrException(tp1.topicPartition())).thenReturn(p1);

+        BiConsumer<SharePartitionKey, Throwable> exceptionHandler = mockExceptionHandler();


For safety, should we be mocking ShareFetchUtils.processFetchResponse for the test testRemoteStorageFetchRequestCompletionOnFutureCompletionFailure

adixitconfluent

Thanks for the changes @brandboat . The PR looks much better now. One small nit.

adixitconfluent · 2025-10-28T06:20:53Z

core/src/test/java/kafka/server/share/DelayedShareFetchTest.java

        LogOffsetSnapshot endOffsetSnapshot = new LogOffsetSnapshot(1, mock(LogOffsetMetadata.class),
-            hwmOffsetMetadata, mock(LogOffsetMetadata.class));
-        Partition partition = mock(Partition.class);
+                hwmOffsetMetadata, mock(LogOffsetMetadata.class));


nit: Indentation (extra spaces)

fixed in bcc30ae

adixitconfluent

LGTM

AndrewJSchofield

lgtm

KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests

2503c74

github-actions bot added core Kafka Broker KIP-932 Queues for Kafka labels Oct 24, 2025

brandboat changed the title ~~KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests~~ (WIP) KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests Oct 24, 2025

brandboat changed the title ~~(WIP) KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests~~ KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests Oct 24, 2025

brandboat commented Oct 24, 2025

View reviewed changes

brandboat added 2 commits October 25, 2025 00:10

Remove unnecessary comments

30a4bea

Remove redundant assertion

a707238

apoorvmittal10 added the ci-approved label Oct 26, 2025

adixitconfluent suggested changes Oct 27, 2025

View reviewed changes

brandboat added 3 commits October 27, 2025 23:37

Address comments

d25ef04

Merge remote-tracking branch 'origin/trunk' into KAFKA-19345

8350c5b

Remove unrelated changes

97dbe98

adixitconfluent suggested changes Oct 28, 2025

View reviewed changes

brandboat added 2 commits October 28, 2025 22:57

fix indention

bcc30ae

Merge remote-tracking branch 'origin/trunk' into KAFKA-19345

c66811c

adixitconfluent approved these changes Oct 29, 2025

View reviewed changes

AndrewJSchofield approved these changes Oct 29, 2025

View reviewed changes

AndrewJSchofield merged commit 6b96273 into apache:trunk Oct 29, 2025
25 checks passed

brandboat deleted the KAFKA-19345 branch October 29, 2025 11:12

KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests #20765

KAFKA-19345: Use ShareFetchUtils mock for DelayedShareFetchTest tests #20765

Uh oh!

Conversation

brandboat commented Oct 24, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandboat Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandboat commented Oct 25, 2025

Uh oh!

brandboat commented Oct 25, 2025

Uh oh!

apoorvmittal10 commented Oct 26, 2025

Uh oh!

adixitconfluent left a comment

Choose a reason for hiding this comment

Uh oh!

adixitconfluent Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adixitconfluent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adixitconfluent left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brandboat commented Oct 24, 2025 •

edited by github-actions bot

Loading

brandboat Oct 24, 2025 •

edited

Loading

adixitconfluent Oct 27, 2025 •

edited

Loading