lianetm commented on PR #21332: URL: https://github.com/apache/kafka/pull/21332#issuecomment-3916889457
Hey @chickenchickenlove ! Sorry for the delay. The current approach on both consumers is that the leave request on close is sent on a best-effort (e.g., not retried), so I'm not sure we can we really guarantee what this test is claiming, right? (e.g, what if the request is lost? or fails for other reasons on the broker side?) On top of that, changing the logic that prioritized metadata request seems very risky. E.g, could we be filling up the max_in_flight with this leave request that we want to allow to go before metadata, so delaying metadata in new unintended ways? Not sure, needs more thought, but I wouldn't change this without a good motivation. I wonder if our option is to test this at different levels, to test what we can guarantee: - `consumer.close` should trigger the actions that are expected to send the leave even when interrupted (enqueue `LeaveGroupOnCloseEvent` for the async, `LeaveGroupRequest` for classic) - not sure if this is covered. If not, we could add tests in this same PR - `LeaveGroupOnCloseEvent` processing should generate the HB to leave - this is tested in the membership mgr transitions on `leaveGroup` I expect - remove this integration test for leave on interrupt :), we cannot really guarantee this imo Then I do think we should review the best-effort approach to send the leave group. Can we do better and retry/wait while there is time? (not to address this interrupt scenario, here there's not much we can do, but to ensure we retry leaving when unsubscribing or close with time maybe). I had filed a jira for this a long time ago actually https://issues.apache.org/jira/browse/KAFKA-15954 , we can maybe resurface that separately. What do you think? Thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
