Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review58495 --- Ship it! Just two minor comments that we can touch up on check-in. core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment99544 To make this even clearer, I would name the arguments. i.e., (bufferSize, numInputs = numConsumers, numOutputs = numProducers) core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment99540 Braces are actually unnecessary for the case block - Joel Koshy On Oct. 24, 2014, 7:56 a.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 24, 2014, 7:56 a.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Addressed Neha and Guzhang's comments. Incorporated Joel and Neha's comments. Incorporated Joel and Neha's comments. Also fixed a potential race where cleanShutdown could execute multiple times if several threads exit abnormally at same time. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 24, 2014, 7:56 a.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description (updated) --- Addressed Guozhang's comments. Addressed Neha and Guzhang's comments. Incorporated Joel and Neha's comments. Incorporated Joel and Neha's comments. Also fixed a potential race where cleanShutdown could execute multiple times if several threads exit abnormally at same time. Diffs (updated) - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
On Oct. 22, 2014, 9:32 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/tools/MirrorMaker.scala, line 271 https://reviews.apache.org/r/26994/diff/1/?file=727975#file727975line271 Is there any value in setting this to true? It seems that just checking if it is false and exiting the process suffices. Setting to true something that is called cleanShutdown, when in fact, it isn't a clean shutdown is confusing to read. Also good to add a FATAL log entry as suggested by Guozhang as well. Guozhang Wang wrote: The boolean is used when the internal threads tries to exist, when it is not set, the threads knows it is closing abnormally and hence goes on to handle it. I agree its name is a bit misleading, and probably we can just name it as isShuttingDown. I'm thinking maybe we can use a separate flag in each thread to indicate whether it exits normally or not. So in the catch clause we set that flag to indicate the thread is exiting abnormally. That might be clearer. - Jiangjie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57907 --- On Oct. 22, 2014, 10:04 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
On Oct. 21, 2014, 10:21 p.m., Guozhang Wang wrote: core/src/main/scala/kafka/tools/MirrorMaker.scala, line 323 https://reviews.apache.org/r/26994/diff/1/?file=727975#file727975line323 Is this change intended? Jiangjie Qin wrote: Yes, it is intended, so that we can make sure each data channel queue will receive a shutdown message. Otherwise 2 messages could go to the same data channel queue. Is this true? Each put will go to the next queue. i.e., Utils.abs(counter.getAndIncrement()) % numConsumers So suppose there are 10 producers and you put 10 shutdown messages. Each of those output queues will get exactly one shutdown message. I have a separate comment on the existing code wrt naming which I will put in the RB directly. - Joel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57680 --- On Oct. 22, 2014, 10:04 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review58049 --- core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98952 Pre-existing naming issue that would be good to fix in this. At first glance, this is extremely confusing and looks wrong. DataChannel is defined as DataChannel(capacity, numProducers, numConsumers) It is instantiated on line 127 as new DataChannel(capacity, numConsumers, numProducers) i.e., it seems as though the arguments are switched. The consumers/producers parameters of the DataChannel are to be interpreted as inputs/outputs and have nothing to do with numConsumers/numProducers in the mirror maker. So, can you rename these fields in the DataChannel? i.e., numConsumers - numOutputs and numProducers - numInputs? - Joel Koshy On Oct. 22, 2014, 10:04 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
On Oct. 22, 2014, 9:32 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/tools/MirrorMaker.scala, line 271 https://reviews.apache.org/r/26994/diff/1/?file=727975#file727975line271 Is there any value in setting this to true? It seems that just checking if it is false and exiting the process suffices. Setting to true something that is called cleanShutdown, when in fact, it isn't a clean shutdown is confusing to read. Also good to add a FATAL log entry as suggested by Guozhang as well. Guozhang Wang wrote: The boolean is used when the internal threads tries to exist, when it is not set, the threads knows it is closing abnormally and hence goes on to handle it. I agree its name is a bit misleading, and probably we can just name it as isShuttingDown. Jiangjie Qin wrote: I'm thinking maybe we can use a separate flag in each thread to indicate whether it exits normally or not. So in the catch clause we set that flag to indicate the thread is exiting abnormally. That might be clearer. I personally think the flag-per-thread is an overkill here: when each thread is about to exist (i.e. in the finally block), all it needs to check is if the whole MM is currently shutdown or not, if it is, then the thread itself knows it exist normally, otherwise log the FATAL and force killing the MM. - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57907 --- On Oct. 22, 2014, 10:04 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
On Oct. 21, 2014, 10:21 p.m., Guozhang Wang wrote: core/src/main/scala/kafka/tools/MirrorMaker.scala, line 323 https://reviews.apache.org/r/26994/diff/1/?file=727975#file727975line323 Is this change intended? Jiangjie Qin wrote: Yes, it is intended, so that we can make sure each data channel queue will receive a shutdown message. Otherwise 2 messages could go to the same data channel queue. Joel Koshy wrote: Is this true? Each put will go to the next queue. i.e., Utils.abs(counter.getAndIncrement()) % numConsumers So suppose there are 10 producers and you put 10 shutdown messages. Each of those output queues will get exactly one shutdown message. I have a separate comment on the existing code wrt naming which I will put in the RB directly. Hi Joel, yes, I think you are right, since there will be no other put during the shutting down pahse, though it is kind of wiered that when we call shutdown for 1 thread and the other one got shutdown... - Jiangjie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57680 --- On Oct. 22, 2014, 10:04 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 23, 2014, 11:20 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description (updated) --- Addressed Guozhang's comments. Addressed Neha and Guzhang's comments. Diffs (updated) - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review58168 --- Ship it! LGTM, one minor thing upon check in. core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment99094 fatal(Consumer thread failure due to , t) - Guozhang Wang On Oct. 23, 2014, 11:20 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 23, 2014, 11:20 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Addressed Neha and Guzhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review58171 --- Ship it! Besides the minor stylistic comment, rest looks good. core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment99103 Minor stylistic comment- if(!isShuttingdown.get()) core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment99104 ditto here - Neha Narkhede On Oct. 23, 2014, 11:20 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 23, 2014, 11:20 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Addressed Neha and Guzhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review58185 --- Ship it! Looks good - can you upload an updated RB incorporating the minor comments? core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment99129 exited - Joel Koshy On Oct. 23, 2014, 11:20 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 23, 2014, 11:20 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Addressed Neha and Guzhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
On Oct. 21, 2014, 10:21 p.m., Guozhang Wang wrote: core/src/main/scala/kafka/tools/MirrorMaker.scala, line 323 https://reviews.apache.org/r/26994/diff/1/?file=727975#file727975line323 Is this change intended? Yes, it is intended, so that we can make sure each data channel queue will receive a shutdown message. Otherwise 2 messages could go to the same data channel queue. - Jiangjie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57680 --- On Oct. 21, 2014, 8:37 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 21, 2014, 8:37 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- make mirror maker exit when one consumer/producer thread exits. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57907 --- core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98797 Is there any value in setting this to true? It seems that just checking if it is false and exiting the process suffices. Setting to true something that is called cleanShutdown, when in fact, it isn't a clean shutdown is confusing to read. Also good to add a FATAL log entry as suggested by Guozhang as well. core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98799 Ditto here. - Neha Narkhede On Oct. 21, 2014, 8:37 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 21, 2014, 8:37 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- make mirror maker exit when one consumer/producer thread exits. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description (updated) --- Addressed Guozhang's comments. Diffs (updated) - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
On Oct. 22, 2014, 9:32 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/tools/MirrorMaker.scala, line 271 https://reviews.apache.org/r/26994/diff/1/?file=727975#file727975line271 Is there any value in setting this to true? It seems that just checking if it is false and exiting the process suffices. Setting to true something that is called cleanShutdown, when in fact, it isn't a clean shutdown is confusing to read. Also good to add a FATAL log entry as suggested by Guozhang as well. The boolean is used when the internal threads tries to exist, when it is not set, the threads knows it is closing abnormally and hence goes on to handle it. I agree its name is a bit misleading, and probably we can just name it as isShuttingDown. - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57907 --- On Oct. 22, 2014, 10:04 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 22, 2014, 10:04 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- Addressed Guozhang's comments. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- make mirror maker exit when one consumer/producer thread exits. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
Re: Review Request 26994: Patch for KAFKA-1719
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/#review57680 --- core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98497 Just a stylish comment: could you group java imports with scala / other lib imports, and leave kafka imports at the top? core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98503 Can we add a FATAL log entry here: Consumer thread existed abnormally, stopping the whole mirror maker? core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98501 Ditto above. core/src/main/scala/kafka/tools/MirrorMaker.scala https://reviews.apache.org/r/26994/#comment98499 Is this change intended? - Guozhang Wang On Oct. 21, 2014, 8:37 p.m., Jiangjie Qin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26994/ --- (Updated Oct. 21, 2014, 8:37 p.m.) Review request for kafka. Bugs: KAFKA-1719 https://issues.apache.org/jira/browse/KAFKA-1719 Repository: kafka Description --- make mirror maker exit when one consumer/producer thread exits. Diffs - core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867 Diff: https://reviews.apache.org/r/26994/diff/ Testing --- Thanks, Jiangjie Qin
