> On March 2, 2015, 11:04 p.m., Jiangjie Qin wrote: > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java, > > line 219 > > <https://reviews.apache.org/r/29467/diff/4/?file=882250#file882250line219> > > > > We probably need to release the caller threads that are waiting on > > flush() at this point. > > Jay Kreps wrote: > Making flush a boolean method that you have to always check to see if > someone called close() in another thead would be a really really really > painful api to use in practice, right? > > I think the issue here is actually what I pointed out in the other > comment, namely that in-flight requests area actually left incomplete when > you call close and hit the forceClose timeout. Any other thread blocking on > these futures would block forever. > > The right solution is just to fail all requests that haven't completed > when forceClose kicks in. This then fullfills the criteria for flush which is > that all the requests are completed or failed.
Yes, I agree that letting flush() return a boolean to just indicate whether someone called close is ugly. I'm thinking maybe we can make the return value to be more useful. The idea of letting flush return a boolean comes when I was writing the mirror maker. When we call flush() followed by a consumer.commitOffsets(), we need to know the result of flush() in order to decide whether to commit offset or not. There might be three cases: 1. flush() succeeded on all batches. 2. flush() failed and some exception were thrown to caller thread (very rare, InterruptedException maybe) 3. flush() failed but are handled by sender thread in send callbacks. For 1), no problem, everybody is happy. For 2), caller thread knows something wrong happened and will not do next task (i.e. commit offsets). For 3), caller thread has no idea about what happened and assumes everthing went well. What I'm doing now is in send callback let the sender thread set a flag for the caller thread to check whether the flush succeeded or not when flush() returns. Otherwise, caller thread cannot decide whether to commit offset or not. I'm thinking if in most cases people care about whether flush succeeded or not, they need to have this inter thread communication. If it is a common requirement, maybe we can let flush() return a boolean. >From API point of view, it is probably OK. If user cares about whether flush >succeeded or not, they check the return value, otherwise they ignore it. Just >like the what we do for send(). - Jiangjie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29467/#review74843 ----------------------------------------------------------- On March 2, 2015, 6:41 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29467/ > ----------------------------------------------------------- > > (Updated March 2, 2015, 6:41 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1660 > https://issues.apache.org/jira/browse/KAFKA-1660 > > > Repository: kafka > > > Description > ------- > > Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 > > Conflicts: > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > > Merge remote-tracking branch 'origin/trunk' into KAFKA-1660 > > > Changing log levels as suggested. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 7397e565fd865214529ffccadd4222d835ac8110 > clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java > 6913090af03a455452b0b5c3df78f266126b3854 > clients/src/main/java/org/apache/kafka/clients/producer/Producer.java > 5b3e75ed83a940bc922f9eca10d4008d67aa37c9 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > ed9c63a6679e3aaf83d19fde19268553a4c107c2 > > Diff: https://reviews.apache.org/r/29467/diff/ > > > Testing > ------- > > existing unit tests passed. > > > Thanks, > > Parth Brahmbhatt > >
