kirktrue commented on PR #16686:
URL: https://github.com/apache/kafka/pull/16686#issuecomment-2423208911
@dajac—thanks for the comment...
> I am not sure about 6). In the old implementation, we send it out even if
interrupted.
Agreed. My apologies for the ambiguous wording.
Let me try rewording point 3:
```diff
3. Issuing the network request to leave the group
> must always be sent, regardless of timeout or interrupt state, and
must be performed on the background thread
```
This should clarify that the request is always sent.
Rewording point 6:
```diff
6. The application thread will not wait for
< the 'leave group' request to be
> confirmation that the 'leave group' request was
sent if so doing would violate tenets 4 or 5
```
That is, ideally the application thread wants to make sure the request was
enqueued before continuing with the close process. However, if the timeout has
expired and/or the thread was interrupted, `close()` will _not_ wait to confirm
the 'leave group' was enqueued.
Does that clarify that point?
> I think that we should also clarify how we handle interruption because
this is actually the major difference between the old and the new
implementation. In the old, we send the request before checking the interrupt
flag whereas we implicitly check it before in the new implementation.
That's right.
The call to `Future.get()` serves as a synchronization barrier so the
application thread can ensure the background thread is in the correct state
before continuing. And that's also why the `close()` method in this PR stores
the initial state of the interrupt flag before clearing it. (The thread's
interrupt state is restored temporarily before invoking the
`ConsumerRebalanceListener`, if needed, which is a hack.)
> This is actually why we saw members leaving properly in our tests with the
old consumer even though close is still interrupted.
Regarding this point specifically, included in this PR is [a new integration
test to validate that the consumer leaves the group on
interrupt](https://github.com/kirktrue/kafka/blob/KAFKA-16985-clear-interrupt-on-consumer-close/core/src/test/scala/integration/kafka/api/PlaintextConsumerTest.scala#L828-L873).
Interestingly, when that test is run on the build infrastructure, the existing
consumer _fails_ to leave the group when interrupted. (When the test is run on
my local box, it passes fine 🤷♂️)
> I suppose that it is a gray area but let’s try to have a clear statement
on it.
Agreed. That's why KAFKA-17519 was created; we need to define the intended
behavior for all these different edge cases.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
