> On April 10, 2015, 4:36 p.m., Joel Koshy wrote: > > clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java, > > line 394 > > <https://reviews.apache.org/r/31850/diff/7/?file=921104#file921104line394> > > > > I was trying to find a case where it wouldn't work, but I think it > > works as required. > > > > - Client thread 1 calls close > > - Client thread 2 calls append _before_ the accumulator is closed and > > reaches at or after line 177 > > - Client thread 1 marks the accumulator as closed > > - Sender thread comes to this point and aborts/clears batches. > > - Client thread 2 allocates and returns a new batch (and decrements the > > appendsInProgress count) > > - Sender thread checks appendInProgress which returns false > > - Which is why we need the additional abortBatches after the loop. > > > > It is tricky though. I'm wondering if the following would work and is > > simpler/clearer: make the post-condition of close be (i) the accumulator > > closed flag is true && (ii) there are no pending appends. > > > > IOW in accumulator.close, set the flag to true and then wait until > > there are no appendsInProgress. Do you think that would work?
Talked with Joel offline, blocking on close has issues if close(0) is called from callback. I added a comment to explain the tricky synchronization. - Jiangjie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31850/#review79677 ----------------------------------------------------------- On April 10, 2015, 10:09 p.m., Jiangjie Qin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31850/ > ----------------------------------------------------------- > > (Updated April 10, 2015, 10:09 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1660 > https://issues.apache.org/jira/browse/KAFKA-1660 > > > Repository: kafka > > > Description > ------- > > A minor fix. > > > Incorporated Guozhang's comments. > > > Modify according to the latest conclusion. > > > Patch for the finally passed KIP-15git status > > > Addressed Joel and Guozhang's comments. > > > rebased on trunk > > > Rebase on trunk > > > Addressed Joel's comments. > > > Addressed Joel's comments > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > b91e2c52ed0acb1faa85915097d97bafa28c413a > 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/RecordAccumulator.java > 0e7ab29a07301309aa8ca5b75b32b8e05ddc3a94 > > clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java > 70954cadb5c7e9a4c326afcf9d9a07db230e7db2 > > clients/src/main/java/org/apache/kafka/common/errors/InterruptException.java > fee322fa0dd9704374db4a6964246a7d2918d3e4 > clients/src/main/java/org/apache/kafka/common/serialization/Serializer.java > c2fdc23239bd2196cd912c3d121b591f21393eab > > clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java > 05e2929c0a9fc8cf2a8ebe0776ca62d5f3962d5c > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > 9811a2b2b1e9bf1beb301138f7626e12d275a8db > > Diff: https://reviews.apache.org/r/31850/diff/ > > > Testing > ------- > > Unit tests passed. > > > Thanks, > > Jiangjie Qin > >
