----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36242/#review90644 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 987) <https://reviews.apache.org/r/36242/#comment143766> 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. clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 1345) <https://reviews.apache.org/r/36242/#comment143767> Only related to this patch because I'm looking at ensureNotClosed -- it's only used in one place and is a trivial check. We could just inline it. - Ewen Cheslack-Postava 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 > >
