> 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. > > Edward Ribeiro wrote: > 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. > > Ashish Singh wrote: > I do not think 'CopyOnWriteArray' will buy us anything here. I think > 'ConcurrentModificationException' made you think that it is caused by > multiple threads trying to access 'assignedPartitions'. However, the issue is > that 'assignedPartitions' is being modified while iterating over it. Hope it > helps. > > Ashish Singh wrote: > Actually I take that back, it will avoid the > 'ConcurrentModificationException', however one can argue its bad to use as > modifications of 'assignedPartitions' could be a lot.
I understood that ConcurrentModificationException was being thrown by changing it while iterating in the for-loop, not necessarily multi-thread intensive. ;) But as modifications of 'assignedPartitions' can be too many then it potentially makes ''CopyOnWriteArraySet'' a no option here. Better to leave the defensive copy for now as you said. - 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 > >
