> On July 28, 2015, 8:07 p.m., Edward Ribeiro wrote: > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java, > > line 87 > > <https://reviews.apache.org/r/36871/diff/5/?file=1023563#file1023563line87> > > > > Sorry for being late to the party (I have been sick for 3 days now). > > > > Well, what I gonna say is more a musing than a a review comment: why > > not replace the vanilla (and non thread safe HashSet) from > > assignedPartitions, and similar fields, by a thread safe Set? > > > > This can be done either by letting assignedPartitions be > > ''CopyOnWriteArraySet'' (has to take care with the computational > > complexity of adding/removing in this collection) or > > ''ConcurrentSkipListSet''? > > > > IMHO, making a defensive copy here is nice for this particular case, > > but it still leave room for concurrent exceptions in the future. Just a > > musing though. I am fine with leaving as it-is now. :) > > Ashish Singh wrote: > Thanks, for the review. However, this is not an issue. Closing it.
That's ok, no problem. But if ''assignedPartitions'' got its type replaced from ''HashSet'' to ''CopyOnWriteArray'' this line could be removed (that is no need to make defensive copying and no concurrent exceptions). Just a thing to keep an eye towards in the next patches, that is, if it's not better to resort to concurrent collections instead of making defensive copies. - Edward ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36871/#review93333 ----------------------------------------------------------- On July 28, 2015, 4:17 p.m., Ashish Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36871/ > ----------------------------------------------------------- > > (Updated July 28, 2015, 4:17 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2381 > https://issues.apache.org/jira/browse/KAFKA-2381 > > > Repository: kafka > > > Description > ------- > > Address review comment > > > Move closing of consumer to finally > > > Add a more specific unit test > > > KAFKA-2381: Possible ConcurrentModificationException while unsubscribing from > a topic in new consumer > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java > 4d9a425201115a66b457b58d670992b279091f5a > > clients/src/test/java/org/apache/kafka/clients/consumer/internals/SubscriptionStateTest.java > 319751c374ccdc7e7d7d74bcd01bc279b1bdb26e > core/src/test/scala/integration/kafka/api/ConsumerTest.scala > 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca > > Diff: https://reviews.apache.org/r/36871/diff/ > > > Testing > ------- > > > Thanks, > > Ashish Singh > >
