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