kirktrue commented on PR #16686: URL: https://github.com/apache/kafka/pull/16686#issuecomment-2305971155
Hi @lianetm 👋 > One high level thought. With this PR we're ignoring the interrupted status temporarily, to allow the close to do what it needs (and ensure it cleanly leaves the group), makes sense. But in that case, we're allowing the close to run with whatever timeout was provided in the api call to close (or the default api timeout). Wouldn't make more sense for this interrupt+close(timeout) to behave exactly like close(0)? (instead of behaving like close(timeout) like it does now). This would be the only change: > > `final Timer closeTimer = wasInterrupted ? time.timer(Duration.ZERO) : time.timer(timeout);` > > Seems like a good compromise between respecting the interrupted status, while also attempting to close cleanly (assuming that close(0) does send the leave, this is why I was suggesting we also add an IT for it). Thoughts? That's a really interesting idea. I'm scratching my head on this one, though, because I'm really not sure the right thing to do. While the documentation for [`Consumer.close()`](https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L1745-L1754) mentions situations in which an `InterruptException` is thrown, it doesn't mention if the thread's interrupt status affects the timeout or not. When calling `Consumer.close(Duration)`, in my opinion, users would likely believe that the timeout they provided would be honored, regardless of the thread's interrupt status. I looked briefly at the `ClassicKafkaConsumer`’s `close()` implementation, and I didn't see anything about altering the timeout. But there's a lot of code behind that API call, so I could have easily missed it. -- 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
