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-18017: Fix call order for HB error and group manager #17805

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

lianetm
Copy link
Contributor

@lianetm lianetm commented Nov 13, 2024

Fix to ensure that the group manager is notified about HB errors only after having propagated the error to the app thread via en ErrorEvent. This is to ensure that the app thread is aware of the error if ever there is an ongoing unsubscribe operation processing background events in the app thread.

@lianetm
Copy link
Contributor Author

lianetm commented Nov 13, 2024

Hello @chia7712 , any chance you could take a look at this small fix? Thanks!

@chia7712
Copy link
Contributor

@lianetm Thanks for the patch. The fix makes sense to me. Could you open a JIRA for it since it's a bug fix? Additionally, should we also fix onFailure? It similarly completes the ongoing unsubscribe before sending an ErrorEvent.

membershipManager().onHeartbeatFailure(exception instanceof RetriableException);

@lianetm lianetm changed the title MINOR: Fix call order for HB error and group manager KAFKA-18017: Fix call order for HB error and group manager Nov 14, 2024
@lianetm
Copy link
Contributor Author

lianetm commented Nov 14, 2024

Hi @chia7712, added the fix and test to the onFailure path too, good catch. Thanks for the review!

@kirktrue kirktrue added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients consumer KIP-848 The Next Generation of the Consumer Rebalance Protocol small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants