-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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-16933: New consumer unsubscribe close commit fixes #16272
Conversation
This reverts commit bfae065.
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java
Show resolved
Hide resolved
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.
Hey @lianetm. I left a few comments
applicationEventHandler.addAndGet(new LeaveOnCloseEvent(calculateDeadlineMs(timer))); | ||
}, | ||
"Failed to send leaveGroup heartbeat with a timeout(ms)=" + timer.timeoutMs(), firstException); | ||
completeQuietly(() -> maybeRevokePartitions(), |
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.
So the point here seems to be, if maybeRevokePartitions
fails, likely due to rebalancelistener, still leave the group.
It looks like a good change to me. I'm just surprised to see that the legacy consumer does not seem to do this? If onPrepareLeave
in AbstractCoordinator
fails, we won't reach maybeLeaveGroup
.
So is this a bug also in the legacy consumer?
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.
You got it right, in this case the legacy consumer won't send a leave group either. We discussed it with @dajac and it seemed right to ensure the new consumer leaves on close (even if the callbacks fail). No changes on the legacy for now though (would require more thought).
@@ -1324,6 +1322,7 @@ void completeQuietly(final Utils.ThrowingRunnable function, | |||
} catch (TimeoutException e) { | |||
log.debug("Timeout expired before the {} operation could complete.", msg); |
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.
Should we update firstException
here?
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.
Totally, good catch.
consumer.close(Duration.ZERO); | ||
} catch (Exception e) { | ||
// best effort to clean up after each test, but may throw (ex. if callbacks where | ||
// throwing errors) |
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 did this before but was asked to not do it, see https://github.com/apache/kafka/pull/15613/files/9fb917e4b1e60f238183c92d1ad3bc2565a7e1ea#r1559907295. That's why I added a "clean-up close" in the tests where close fails, with an expected exception (search for "clean-up" in this file).
I'd also be fine with your (and my original approach) to have a best-effort clean up and ignore exceptions here. But then, let's remove the "clean-up close" code in the other tests. Any consistent approach is fine with me here.
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.
Oh I see, good point. Agree with going with a consistent approach here, but could I bring it in a follow-up PR right after this? (it wouldn't need to get into 3.8). The trick is that now the close may throw for any of the tests that throw on the callback, so I would prefer to play safe, make sure the close does not throw here, and I'll review all the tests that we expect could rightfully throw on close now (Also the other clean-up logic has some Timeout related checks so better not removing blindly, could have some value it seems)
// Clear the subscription, no matter if the callback execution failed or succeeded. | ||
subscriptions.unsubscribe(); | ||
clearSubscription(); |
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.
Is the name clearSubscription
misleading? It seems like it clears the assignment, not the subscription.
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.
you're right, it's clearing assignments, renamed.
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.
Sorry, didn't mean to approve yet
Hey @lucasbru, all comments addressed. Thanks for the review! |
Hey @lucasbru , note I included this small commit 56ac969 we realized we needed to avoid the race on unsubscribe/close. I will follow up on another PR with the fix to avoid getting to a revocation of partitions that are not assigned on close, but still having the sanity check at the low level seems a safe bet while the root cause is fixed. cc. @dajac |
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.
LGTM. I'll wait for the tests to pass
Build completed with 5 unrelates failures:
|
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.
hi @lianetm thanks for the pr and fixing the logs and issues. I guess we will file a separated issue to address the interrupted exception weve see during testing.
Fixes for the leave group flow (unsubscribe/close): Fix to send Heartbeat to leave group on close even if the callbacks fail fix to ensure that if a member gets fenced while blocked on callbacks (ex. on unsubscribe), it will clear its epoch to not include it in commit requests fix to avoid race on the subscription state object on unsubscribe, updating it only on the background thread when the callbacks to leave complete (success or failure). Also improving logging in this area. Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Philip Nee <pnee@confluent.io>
Test failures are unrelated Merged to trunk and cherrypicked to 3.8 cc @jlprat |
Hey @philipnee, agreed there is still an issue with the Interrupt discovered with the stress tests, it's just that as discussed offline that will come in a separate PR (just to not keep stretching this one). Just for the record, there is also another issue similar to the one fixed on this PR for the subscription state https://issues.apache.org/jira/browse/KAFKA-16954. I plan to open a PR today with both fixes for this related blocker. cc. @lucasbru , @jlprat, @dajac . Thanks! |
Fixes for the leave group flow (unsubscribe/close):
Also improving logging in this area.