Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/
---

(Updated July 28, 2015, 12:56 a.m.)


Review request for kafka.


Bugs: KAFKA-2381
https://issues.apache.org/jira/browse/KAFKA-2381


Repository: kafka


Description
---

KAFKA-2381: Possible ConcurrentModificationException while unsubscribing from a 
topic in new consumer


Diffs (updated)
-

  
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java
 4d9a425201115a66b457b58d670992b279091f5a 
  core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 

Diff: https://reviews.apache.org/r/36871/diff/


Testing
---


Thanks,

Ashish Singh

Re: Review Request 36871: Patch for KAFKA-2381

[Jason Gustafson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93213
---


Ouch. Hard to believe this wasn't caught yet.


core/src/test/scala/integration/kafka/api/ConsumerTest.scala (line 220)


Could we catch this issue more directly with a unit test for 
SubscriptionState?


- Jason Gustafson


On July 28, 2015, 12:56 a.m., Ashish Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36871/
> ---
> 
> (Updated July 28, 2015, 12:56 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2381
> https://issues.apache.org/jira/browse/KAFKA-2381
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> 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 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 
> 
> Diff: https://reviews.apache.org/r/36871/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashish Singh
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Aditya Auradkar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93215
---



core/src/test/scala/integration/kafka/api/ConsumerTest.scala (line 233)


consider closing this in a finally. A failing test can cause incorrect tear 
down of the test


- Aditya Auradkar


On July 28, 2015, 12:56 a.m., Ashish Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36871/
> ---
> 
> (Updated July 28, 2015, 12:56 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2381
> https://issues.apache.org/jira/browse/KAFKA-2381
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> 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 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 
> 
> Diff: https://reviews.apache.org/r/36871/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ashish Singh
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/
---

(Updated July 28, 2015, 4:56 a.m.)


Review request for kafka.


Bugs: KAFKA-2381
https://issues.apache.org/jira/browse/KAFKA-2381


Repository: kafka


Description (updated)
---

Add a more specific unit test


KAFKA-2381: Possible ConcurrentModificationException while unsubscribing from a 
topic in new consumer


Diffs (updated)
-

  
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

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/
---

(Updated July 28, 2015, 4:59 a.m.)


Review request for kafka.


Bugs: KAFKA-2381
https://issues.apache.org/jira/browse/KAFKA-2381


Repository: kafka


Description (updated)
---

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 (updated)
-

  
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

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

> On July 28, 2015, 1:15 a.m., Aditya Auradkar wrote:
> > core/src/test/scala/integration/kafka/api/ConsumerTest.scala, line 233
> > 
> >
> > consider closing this in a finally. A failing test can cause incorrect 
> > tear down of the test

Done. Thanks for the review.


- Ashish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93215
---


On July 28, 2015, 4:59 a.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:59 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2381
> https://issues.apache.org/jira/browse/KAFKA-2381
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> 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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

> On July 28, 2015, 1:11 a.m., Jason Gustafson wrote:
> > Ouch. Hard to believe this wasn't caught yet.

It is. Thanks for the review. Addressed your concern.


- Ashish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93213
---


On July 28, 2015, 4:59 a.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:59 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2381
> https://issues.apache.org/jira/browse/KAFKA-2381
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> 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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ismael Juma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93256
---



clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java
 (line 86)


A minor optimisation is to use `ArrayList` here to avoid rehashing the 
items in the list.


- Ismael Juma


On July 28, 2015, 4:59 a.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:59 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2381
> https://issues.apache.org/jira/browse/KAFKA-2381
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> 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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

---
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 (updated)
---

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 (updated)
-

  
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

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

> On July 28, 2015, 8:16 a.m., Ismael Juma wrote:
> > clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java,
> >  line 86
> > 
> >
> > A minor optimisation is to use `ArrayList` here to avoid rehashing the 
> > items in the list.

Fair point, addressed.


- Ashish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93256
---


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ismael Juma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93306
---

Ship it!


Code change looks good. Not very familiar with the tests, so can't say if this 
is the best way to do it (seems OK though).

- Ismael Juma


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Jason Gustafson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93321
---

Ship it!


Ship It!

- Jason Gustafson


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Edward Ribeiro]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review9
---



clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java
 (line 87)


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. :)


- Edward Ribeiro


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

> 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
> > 
> >
> > 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. :)

Thanks, for the review. However, this is not an issue. Closing it.


- Ashish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review9
---


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Edward Ribeiro]

> 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
> > 
> >
> > 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/#review9
---


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

> 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
> > 
> >
> > 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.

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


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review9
---


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Ashish Singh]

> 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
> > 
> >
> > 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.

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.


- Ashish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review9
---


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Guozhang Wang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36871/#review93351
---

Ship it!


Ship It!

- Guozhang Wang


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
> 
>

Re: Review Request 36871: Patch for KAFKA-2381

[Edward Ribeiro]

> 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
> > 
> >
> > 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/#review9
---


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
> 
>