KAFKA-16684: Remove cache in responseData by m1a2st · Pull Request #16532 · apache/kafka

m1a2st · 2024-07-05T14:28:48Z

The response data should change accordingly to the input, however with the current design, it will not change even if the input changes. We should remove this cache logic to avoid returning wrong data.
Jira: https://issues.apache.org/jira/browse/KAFKA-16684

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

chia7712

LGTM if it does not hurt CI

chia7712 · 2024-07-05T21:43:12Z

    }
+
+    @Test
+    public void testFetcherDontCacheAnyData() {


This part is good but it's gone too far. Maybe we can create a FetchResponse and then test the method responseData?

Yes, I will simplify this test

m1a2st · 2024-07-06T12:04:11Z

@chia7712, Thanks for your comments, PTAL

chia7712 · 2024-07-07T17:17:33Z

+        FetchResponse fetchResponse = fetchResponse(tidp0, records, Errors.NONE, 100L, -1L, 0L, 0);
+        fetchResponse.responseData(topicNames, version)
+                .forEach((topicPartition, partitionData) -> assertEquals(records, partitionData.records()));
+        fetchResponse.responseData(new HashMap<>(), version)


Collections.emptyMap()

m1a2st · 2024-07-07T22:26:23Z

@chia7712, Thanks for your comments, PTAL

chia7712

@m1a2st thanks for updated patch. one last comment is left

chia7712 · 2024-07-08T09:42:30Z

+    public void testFetcherDontCacheAnyData() {
+        short version = 17;
+        FetchResponse fetchResponse = fetchResponse(tidp0, records, Errors.NONE, 100L, -1L, 0L, 0);
+        fetchResponse.responseData(topicNames, version)


Could you please verify the size first?

m1a2st · 2024-07-08T09:50:37Z

@chia7712, Thanks for your comments, add assert for size()

chia7712 · 2024-07-08T19:28:28Z

@apoorvmittal10 you had reviewed on #15966, so could you please take a look at this PR? thanks!

apoorvmittal10

Thanks for the PR. LGTM! Just a query.

apoorvmittal10 · 2024-07-08T21:31:20Z

+
+    @Test
+    public void testFetcherDontCacheAnyData() {
+        short version = 17;


Do we have any existing test where version < 13? If not then can we please add one.

@apoorvmittal10, Thanks for your comments, In testFetchWithNoTopicId have been test for version 12, Should I add a test only for FetchResponse#responseData this method to test version 12? WDYT

Confirmed, org.apache.kafka.clients.consumer.internals.FetcherTest#testFetchWithNoTopicId is testing org.apache.kafka.common.requests.FetchResponse#responseData with version = 12, so I don't think we need a new test.

Thansk for looking into, if there already exists a test then we can skip new one.

soarez

Thanks for this patch.

I could not find a good reason to keep the reponseData cached.

Test failures are unrelated.

LGTM

The response data should change accordingly to the input, however with the current design, it will not change even if the input changes. We remove this cache logic to avoid returning wrong data. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Apoorv Mittal <apoorvmittal10@gmail.com>, Igor Soarez <soarez@apple.com>

FrankYang0529 · 2025-04-04T14:58:33Z

+        responseData.forEach((topicPartition, partitionData) -> assertEquals(records, partitionData.records()));
+        LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> nonResponseData = fetchResponse.responseData(emptyMap(), version);
+        assertEquals(emptyMap().size(), nonResponseData.size());
+        nonResponseData.forEach((topicPartition, partitionData) -> assertEquals(MemoryRecords.EMPTY, partitionData.records()));


This line is never executed. We can remove this assertion.

we can also replace emptyMap().size() with 0.

I've opened a minor PR #19376 to remove it.

Thanks @FrankYang0529 for pointing it out!

…19357) Jira: https://issues.apache.org/jira/browse/KAFKA-19074 Similar fix #16532 2b8aff5 make it accept input to return "partial" data. The content of output is based on the input but we cache the output ... It will return same output even though we pass different input. That is a potential bug. Reviewers: PoAn Yang <payang@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>

This is from [#16532's comment](https://github.com/apache/kafka/pull/16532/files#r2028985028): The forEach loop in the assertion will never execute because `nonResponseData` is empty. This happens because the above assertion `emptyMap()` has a size of 0, so there are no elements to iterate over. Reviewers: PoAn Yang <payang@apache.org>, Ken Huang <s7133700@gmail.com>, TaiJuWu <tjwu1217@gmail.com>, TengYao Chi <kitingiao@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

m1a2st added 2 commits July 5, 2024 08:33

remove FetchResponse cache

e32700b

add new test for fetcher

e396bcd

chia7712 approved these changes Jul 5, 2024

View reviewed changes

chia7712 reviewed Jul 5, 2024

View reviewed changes

simplify cache test

2eecce2

chia7712 reviewed Jul 7, 2024

View reviewed changes

change to use Collections.emptyMap()

a3ace40

chia7712 reviewed Jul 8, 2024

View reviewed changes

assert size first

12784e1

apoorvmittal10 approved these changes Jul 8, 2024

View reviewed changes

soarez approved these changes Jul 9, 2024

View reviewed changes

soarez merged commit 5608b5c into apache:trunk Jul 9, 2024

TaiJuWu mentioned this pull request Apr 3, 2025

KAFKA-19074: Remove the cached responseData from ShareFetchResponse #19357

Merged

FrankYang0529 reviewed Apr 4, 2025

View reviewed changes

chia7712 mentioned this pull request Apr 4, 2025

MINOR: Clean up unreachable code in FetcherTest #19376

Merged

Conversation

m1a2st commented Jul 5, 2024

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 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!

m1a2st commented Jul 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st commented Jul 7, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st commented Jul 8, 2024

Uh oh!

chia7712 commented Jul 8, 2024

Uh oh!

apoorvmittal10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1a2st Jul 9, 2024 • 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!

soarez 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!

Choose a reason for hiding this comment

Uh oh!

Parkerhiphop Apr 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

m1a2st Jul 9, 2024 •

edited

Loading

Parkerhiphop Apr 4, 2025 •

edited

Loading