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

johnnychhsu · 2024-05-15T15:10:28Z

Context

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 · 2024-05-19T10:24:52Z

+                        responseDataTmp.put(new TopicPartition(name, partition.partitionIndex()), partition));
+                }
+            });
+            responseData = responseDataTmp;


This is what I do concern! The responseData could be different if the input gets changed.

thanks for the review!
yes, the returned data should be calculated on the fly based on the input topic names

chia7712 · 2024-06-06T19:49:46Z

@johnnychhsu This is a bug fix, so please add test for it

TaiJuWu

LGTM

apoorvmittal10

LGTM! The only thing which I am not sure is that do we have any reason to cache the response for faster processing i.e. how frequently this method is called on same topicNames? If that call is too frequesnt with no change in topicNames then will it be worth to re-compute only if topicNames have changed?

The reason I say that is because the method call happens while fetching topicNames from session, which mostly will remain same.

chia7712 · 2024-07-02T15:41:20Z

The reason I say that is because the method call happens while fetching topicNames from session, which mostly will remain same.

that is a good point. Maybe the cache should be hosted by session instead of response data. the response data can be accessed by everyone, so it seems to me that is not a good choice to have cache.

apoorvmittal10 · 2024-07-02T16:28:42Z

The reason I say that is because the method call happens while fetching topicNames from session, which mostly will remain same.

that is a good point. Maybe the cache should be hosted by session instead of response data. the response data can be accessed by everyone, so it seems to me that is not a good choice to have cache.

Sounds fair 👍

chia7712 · 2024-07-02T16:36:05Z

Sounds fair

I file https://issues.apache.org/jira/browse/KAFKA-17065 to log it

m1a2st · 2024-07-03T06:50:23Z

LGTM, I wll add test after this PR merge into trunk.

chia7712 · 2024-07-03T18:40:55Z

org.gradle.api.internal.tasks.testing.TestSuiteExecutionException: Could not complete execution for Gradle Test Executor 100.
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:64)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:119)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:66)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: java.lang.OutOfMemoryError: GC overhead limit exceeded
	at net.bytebuddy.description.type.TypeDescription$ForLoadedType.of(TypeDescription.java:8619)
	at net.bytebuddy.description.method.MethodDescription$ForLoadedMethod.getDeclaringType(MethodDescription.java:1190)
	at org.mockito.internal.creation.bytebuddy.MockMethodAdvice.isOverridden(MockMethodAdvice.java:199)
	at org.apache.kafka.clients.consumer.internals.ConsumerNetworkClient.isUnavailable(ConsumerNetworkClient.java:560)
	at org.apache.kafka.clients.consumer.internals.Fetcher.isUnavailable(Fetcher.java:87)
	at org.apache.kafka.clients.consumer.internals.AbstractFetch.prepareFetchRequests(AbstractFetch.java:427)
	at org.apache.kafka.clients.consumer.internals.Fetcher.sendFetches(Fetcher.java:105)
	at org.apache.kafka.clients.consumer.internals.FetcherTest.sendFetches(FetcherTest.java:246)
	at org.apache.kafka.clients.consumer.internals.FetcherTest.testFetcherConcurrency(FetcherTest.java:2943)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:728)

the failed test is related to this PR. In the test case testFetcherConcurrency, it does not return correct sessionTopicNames so normally it should NOT see the correct response data. However, FetchResponse#responseData will return the cached data regardless of input, so it CAN get correct response data even though it pass empty topicNames. That is a good example of showing the potential bug :)

@m1a2st Could you copy the changes of this PR to another one, and please fix testFetcherConcurrency according to my comment. Also, please add new test for the change.

@johnnychhsu Sorry, I can't merge this PR as it causes the failed test. Please feel free to close this PR as @m1a2st will leverage this PR to complete it :)

m1a2st · 2024-07-03T23:55:49Z

@chia7712, Thanks for your comments, I will open new PR for this issue.

chia7712 · 2024-07-04T11:52:15Z

close this PR @m1a2st will file another one

johnnychhsu marked this pull request as ready for review May 15, 2024 15:19

chia7712 reviewed May 19, 2024

View reviewed changes

Johnny Hsu added 2 commits June 6, 2024 20:25

remove cache in responseData

80c7995

remove sync check

1291dd2

johnnychhsu force-pushed the KAFKA-16684/remove_cache branch from dfadc1a to 1291dd2 Compare June 6, 2024 12:41

Johnny Hsu added 2 commits June 6, 2024 20:42

rename parameters

a67cc66

fix error

707762d

TaiJuWu approved these changes Jun 14, 2024

View reviewed changes

chia7712 mentioned this pull request Jun 14, 2024

KAFKA-16713: Define initial set of RPCs for KIP-932 #16022

Merged

3 tasks

apoorvmittal10 approved these changes Jul 2, 2024

View reviewed changes

m1a2st approved these changes Jul 3, 2024

View reviewed changes

chia7712 closed this Jul 4, 2024

chia7712 mentioned this pull request Jul 8, 2024

KAFKA-16684: Remove cache in responseData #16532

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-16684: Remove cache in responseData#15966

KAFKA-16684: Remove cache in responseData#15966
johnnychhsu wants to merge 4 commits intoapache:trunkfrom
johnnychhsu:KAFKA-16684/remove_cache

johnnychhsu commented May 15, 2024

Uh oh!

chia7712 May 19, 2024

Uh oh!

johnnychhsu Jun 6, 2024

Uh oh!

chia7712 commented Jun 6, 2024

Uh oh!

TaiJuWu left a comment

Uh oh!

apoorvmittal10 left a comment

Uh oh!

chia7712 commented Jul 2, 2024

Uh oh!

apoorvmittal10 commented Jul 2, 2024

Uh oh!

chia7712 commented Jul 2, 2024

Uh oh!

m1a2st commented Jul 3, 2024

Uh oh!

chia7712 commented Jul 3, 2024

Uh oh!

m1a2st commented Jul 3, 2024

Uh oh!

chia7712 commented Jul 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

johnnychhsu commented May 15, 2024

Context

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 May 19, 2024

Choose a reason for hiding this comment

Uh oh!

johnnychhsu Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jun 6, 2024

Uh oh!

TaiJuWu left a comment

Choose a reason for hiding this comment

Uh oh!

apoorvmittal10 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 commented Jul 2, 2024

Uh oh!

apoorvmittal10 commented Jul 2, 2024

Uh oh!

chia7712 commented Jul 2, 2024

Uh oh!

m1a2st commented Jul 3, 2024

Uh oh!

chia7712 commented Jul 3, 2024

Uh oh!

m1a2st commented Jul 3, 2024

Uh oh!

chia7712 commented Jul 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants