Hi Ted, Thanks for the review.
During phase #2, the consumer continuously polls until it either gets some responses or spends the whole timeout. If it reaches the timeout, it doesn't mean that the operation "timed out" in an exceptional way, it just means we determined that there are no records available for our consumer. Throwing when we reach the timeout wold make the method contract "Returns a non-empty collection or throws a TimeoutException". Compared to phase #1, in which a timeout means we actually didn't get to do the requested operation (get an assignment), it seems like returning an empty collection is appropriate when our observation is that there are no records to process. Throwing an exception also means that any consumer that is keeping updated is going to throw an exception on most calls to poll, which suggests the condition really isn't that exceptional. I agree that it's a little odd to have this caveat, and I actually was thinking of the opposite approach: never throwing an exception and always returning an empty collection when it can't finish in the timeout. But I'm worried that folks may not deeply read the release notes and realize they need to change their calling behavior w.r.t. poll. Just returning an empty collection may lead them to believe that (like today) after the call to poll, the consumer has an assignment, which would no longer be correct. So throwing an exception in this case is mostly ensuring callers don't have their expectations silently broken. Thanks, -John On Tue, Apr 17, 2018 at 1:22 PM, Ted Yu <yuzhih...@gmail.com> wrote: > bq. This method will continue to return an empty collection if it doesn't > complete phase #2 within the timeout. If phase #1 times out, poll will > throw a TimeoutException. > > It seems response from the new poll method depends on which phase it is in > prior to the timeout. > Wouldn't it be cleaner if TimeoutException is thrown regardless of the > phase it is in ? > > Cheers > > On Tue, Apr 17, 2018 at 11:08 AM, John Roesler <j...@confluent.io> wrote: > > > Hello all, > > > > I am proposing KIP-288 to improve the semantics of Consumer.poll() w.r.t. > > the timeout parameter and to add a new method for blocking for assignment > > only. > > > > Please find the details here: > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > 288%3A+Consumer.poll%28%29+timeout+semantic+change+and+ > > new+waitForAssignment+method > > > > Please let me know what you think! > > > > Thanks, > > -John > > >
