> On July 7, 2015, 5:31 a.m., Ewen Cheslack-Postava wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, > > line 987 > > <https://reviews.apache.org/r/36242/diff/1/?file=1000884#file1000884line987> > > > > This check could even be removed -- if acquire() succeeded, then this > > would only return if there was incorrect multi-threaded access. > > > > Actually, now that I think about it more, it seems like maybe we > > changed the semanitcs of close() a bit. Clearly it was previously intended > > to be safe to call multiple times, but acquire() will throw if it's already > > been closed. KafkaProducer doesn't have the same type of check, although > > I'm not certain what happens if you invoke close twice there (and it also > > has a different threading model). > > > > I think it might be better to not allow double closing this, so I think > > I'd just remove the check and leave the possibility that acquire() causes > > close() to throw an exception.
I agree with Ewen that since the KafkaConsumer is expecting single-threaded access, it would be possibly OK to reject multiple calls to close. In KafkaProducer multiple calls to close() is allowed since it is designed for shared-thread access. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36242/#review90644 ----------------------------------------------------------- On July 7, 2015, 4:29 a.m., Tim Brooks wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36242/ > ----------------------------------------------------------- > > (Updated July 7, 2015, 4:29 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2311 > https://issues.apache.org/jira/browse/KAFKA-2311 > > > Repository: kafka > > > Description > ------- > > Make closed flag atomic on consumer > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > 1f0e51557c4569f0980b72652846b250d00e05d6 > > Diff: https://reviews.apache.org/r/36242/diff/ > > > Testing > ------- > > > Thanks, > > Tim Brooks > >
