You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jiangjie Qin <be...@gmail.com> on 2014/10/21 22:37:04 UTC
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
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
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
Posted by Joel Koshy <jj...@gmail.com>.
> 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
Posted by Jiangjie Qin <be...@gmail.com>.
> 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
Posted by Jiangjie Qin <be...@gmail.com>.
> 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
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 26994: Patch for KAFKA-1719
Posted by Jiangjie Qin <be...@gmail.com>.
> 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
Posted by Guozhang Wang <gu...@linkedin.com>.
> 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
>
>
Re: Review Request 26994: Patch for KAFKA-1719
Posted by Guozhang Wang <gu...@linkedin.com>.
> 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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
>
>