-
Notifications
You must be signed in to change notification settings - Fork 14.9k
MINOR : Tolerate GroupIdNotFoundException in consumer when leaving a group. #21239
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
MINOR : Tolerate GroupIdNotFoundException in consumer when leaving a group. #21239
Conversation
|
@ShivsundarR I still get a failure of |
|
Thanks @AndrewJSchofield, I was using the incorrect state for the check, it should actually be |
AndrewJSchofield
left a comment
There was a problem hiding this 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. I think the revised member state in the check is now correct. A few very minor comments only.
| heartbeatRequestName(), error, errorMessage); | ||
| handleFatalFailure(error.exception(errorMessage)); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's have a blank line following the break to match the other cases in this switch.
| membershipManager().onHeartbeatRequestSkipped(); | ||
| } else { | ||
| // Else, this is a fatal error, we should throw it and transition to fatal state. | ||
| logger.error("{} failed due to unexpected error {}: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The following logger.error is identical but on a single line. Please change this to a single line also.
| // from LEAVING to UNSUBSCRIBED in onHeartbeatRequestGenerated() before the request is sent. | ||
| if (membershipManager().state() == MemberState.UNSUBSCRIBED) { | ||
| logger.info("{} received GROUP_ID_NOT_FOUND for group {} while unsubscribed. " + | ||
| "Not treating as fatal since consumer is leaving group.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the "Not treating as fatal since consumer is leaving group.". There's no sense in using "fatal" in an information log line. This is nothing to worry about in the slightest.
…group. (apache#21239) *What* - Currently if a consumer/share-consumer calls `close()` before it has joined a group, then the heartbeat on close will be sent with `epoch` = -1 and the broker would return "`GroupIdNotFoundException`". - This was causing couple of tests in `ShareConsumerTest` to be flaky if the heartbeat to join the group was sent with `epoch` = -1. - Since this can occur in real scenarios as well, it would be better to tolerate this exception while we are leaving the group so that the consumer can close cleanly. Reviewers: Andrew Schofield <[email protected]>
What
close()before it hasjoined a group, then the heartbeat on close will be sent with
epoch=-1 and the broker would return "
GroupIdNotFoundException".ShareConsumerTestto be flaky ifthe heartbeat to join the group was sent with
epoch= -1.tolerate this exception while we are leaving the group so that the
consumer can close cleanly.
Reviewers: Andrew Schofield [email protected]