Sorry - I dug a bit into the old PR. Seems like the issue is there's broken contract that if the commitSync won't wait for the previous async commits to complete if it tries to commit an empty offset map.
On Tue, May 2, 2023 at 12:49 PM Philip Nee <philip...@gmail.com> wrote: > Hey Erik, > > Just a couple of questions to you: Firstly, could you explain the > situation in that you would prefer to invoke commitAsync over commitSync in > the rebalance listener? Typically we would use the synchronized method to > ensure the commits are completed before moving on with the rebalancing, > which leads to my second comment/question. is it your concern that we > currently don't have a way to invoke the callback, and the user won't be to > correctly handle these failed/successful async commits? > > Thanks, > P > > On Tue, May 2, 2023 at 12:22 PM Erik van Oosten > <e.vanoos...@grons.nl.invalid> wrote: > >> Dear developers of the Kafka java client, >> >> It seems I have found a feature gap in the Kafka java client. >> KAFKA-10337 and its associated pull request on Github (from 2020!) would >> solve this, but it was closed without merging. We would love to see it >> being reconsidered for merging. This mail has the arguments for doing so. >> >> The javadoc of `ConsumerRebalanceListener` method `onPartitionsRevoked` >> recommends you commit all offsets within the method, thereby holding up >> the rebalance until those commits are done. The (perceived) feature gap >> is when the user is trying to do async commits from the rebalance >> listener; there is nothing available to trigger the callbacks of >> completed commits. Without these callback, there is no way to know when >> it is safe to return from onPartitionsRevoked. (We cannot call `poll` >> because the rebalance listener is already called from inside a poll.) >> >> Calling `commitAsync` with an empty offsets parameter seems a perfect >> candidate for triggering callbacks of earlier commits. Unfortunately, >> commitAsync doesn't behave that way. This is fixed by mentioned pull >> request. >> >> The pull request conversation has a comment saying that calling `commit` >> with an empty offsets parameter is not something that should happen. I >> found this a strange thing to say. First of all, the method does have >> special handling for this situation, negating the comment outright. In >> addition this special handling violates the contract of the method (as >> specified in the javadoc section about ordering). Therefore, this pull >> request has 2 advantages: >> >> 1. KafkaConsumer.commitAsync will be more in line with its javadoc, >> 2. the feature gap is gone. >> >> Of course, it might be that I missed something and that there are other >> ways to trigger the commit callbacks. I would be very happy to hear >> about that because it means I do not have to wait for a release cycle. >> >> If you agree these arguments are sound, I would be happy to make the >> pull request mergable again. >> >> Curious to your thoughts and kind regards, >> Erik. >> >> >> -- >> Erik van Oosten >> e.vanoos...@grons.nl >> https://day-to-day-stuff.blogspot.com >> Committer of zio-kafkahttps://github.com/zio/zio-kafka >> >