Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-18397: Added null check before sending background event from ShareConsumeRequestManager. #18419

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

ShivsundarR
Copy link
Contributor

What
Added a null check for acknowledgements before sending background event from ShareConsumeRequestManager.
Added a unit test which verifies the change.

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

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The code change looks good to me, but I have a couple of comments about the testing.

// For commitAsync, we do not wait for other results to complete, we prepare a background event
// for every ShareAcknowledgeResponse.
// For commitAsync, we send out a background event for every TopicIdPartition, so we use a singletonMap each time.
if (isCommitAsync) {
maybeSendShareAcknowledgeCommitCallbackEvent(Collections.singletonMap(partition, acknowledgements));
if (acknowledgements != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Please could we have a test on the ResultHandler class directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense, I have added tests now for that. Thanks.

@AndrewJSchofield AndrewJSchofield merged commit 3c7ed33 into apache:trunk Jan 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants