Skip to content

Conversation

@TaiJuWu
Copy link
Collaborator

@TaiJuWu TaiJuWu commented Apr 3, 2025

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

@github-actions github-actions bot added triage PRs from the community core Kafka Broker clients small Small PRs labels Apr 3, 2025
@AndrewJSchofield AndrewJSchofield added KIP-932 Queues for Kafka ci-approved and removed triage PRs from the community labels Apr 3, 2025
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Leave a minor comment.


LinkedHashMap<TopicIdPartition, ShareFetchResponseData.PartitionData> nonResponseData = shareFetch.responseData(Map.of());
assertEquals(0, nonResponseData.size());
nonResponseData.forEach((topicIdPartition, partitionData) -> assertEquals(MemoryRecords.EMPTY, partitionData.records()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nonResponseData size is 0, the assertion for data inside it will not be executed. Do we still need this line? Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it, thanks!

private static final int BATCH_SIZE = 500;

private final TopicIdPartition tidp0 = new TopicIdPartition(Uuid.randomUuid(), 0, "topic");
private MemoryRecords records;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two variables are only used in testDontCacheAnyData. Could we move them into the case, so we don't need to build records for each test case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original thought was we can leverage this variable in future like FetcherTest to add more tests but I am also fine to move this variable into method at the moment.

@chia7712 chia7712 merged commit ebb6281 into apache:trunk Apr 5, 2025
23 checks passed
@TaiJuWu TaiJuWu deleted the KAFKA-19074-shareFetch branch April 5, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved clients core Kafka Broker KIP-932 Queues for Kafka small Small PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants