Re: Review Request 20745: Patch for KAFKA-1397

2014-05-05 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review42058 --- core/src/main/scala/kafka/controller/TopicDeletionManager.scala

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-05 Thread Timothy Chen
On May 5, 2014, 5:12 p.m., Jun Rao wrote: core/src/main/scala/kafka/utils/ShutdownableThread.scala, lines 28-33 https://reviews.apache.org/r/20745/diff/6/?file=573313#file573313line28 I thought you plan to remove isShuttingDown? Odd I don't have this anymore in my box, but end up

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-05 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review42195 --- core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-05 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review42199 --- core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-05 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated May 5, 2014, 9 p.m.) Review request for kafka. Bugs: KAFKA-1397

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-02 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review42015 --- Not all comments from the previous round of review have been

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-02 Thread Timothy Chen
On May 2, 2014, 12:26 a.m., Jun Rao wrote: core/src/main/scala/kafka/utils/ShutdownableThread.scala, lines 35-36 https://reviews.apache.org/r/20745/diff/5/?file=573213#file573213line35 We probably should check the return value of initiateShutdown(). I was thinking that even with

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-02 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated May 2, 2014, 8:38 p.m.) Review request for kafka. Bugs: KAFKA-1397

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-01 Thread Timothy Chen
On May 1, 2014, 6:26 p.m., Jun Rao wrote: core/src/main/scala/kafka/controller/TopicDeletionManager.scala, lines 394-396 https://reviews.apache.org/r/20745/diff/4/?file=571881#file571881line394 It seems this test is not necessary since the outer loop will detect that too.

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-01 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated May 1, 2014, 10:54 p.m.) Review request for kafka. Bugs: KAFKA-1397

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-01 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41979 --- core/src/main/scala/kafka/admin/AdminUtils.scala

Re: Review Request 20745: Patch for KAFKA-1397

2014-05-01 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated May 2, 2014, 1:12 a.m.) Review request for kafka. Bugs: KAFKA-1397

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-30 Thread Timothy Chen
On April 29, 2014, 5:02 p.m., Jun Rao wrote: core/src/main/scala/kafka/controller/ControllerChannelManager.scala, lines 209-212 https://reviews.apache.org/r/20745/diff/3/?file=569935#file569935line209 Is it better to do the check here or in the caller? Hmm, at first glance not

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-30 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated April 30, 2014, 9:55 p.m.) Review request for kafka. Bugs:

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-29 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41714 --- Some comments: 1. Should we restore the commented out test in

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-29 Thread Jun Rao
On April 28, 2014, 7:43 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line 210 https://reviews.apache.org/r/20745/diff/1/?file=569160#file569160line210 better to not leak the logic from delete topic manager here. It is worth

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-28 Thread Neha Narkhede
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/#review41626 ---

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-28 Thread Timothy Chen
On April 28, 2014, 7:43 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/controller/ControllerChannelManager.scala, line 210 https://reviews.apache.org/r/20745/diff/1/?file=569160#file569160line210 better to not leak the logic from delete topic manager here. It is worth

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-28 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated April 28, 2014, 9:48 p.m.) Review request for kafka. Bugs:

Re: Review Request 20745: Patch for KAFKA-1397

2014-04-28 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- (Updated April 29, 2014, 12:08 a.m.) Review request for kafka. Bugs:

Review Request 20745: Patch for KAFKA-1397

2014-04-27 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20745/ --- Review request for kafka. Bugs: KAFKA-1397