> On July 10, 2015, 4:46 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/admin/TopicCommand.scala, lines 72-73 > > <https://reviews.apache.org/r/34403/diff/4/?file=1008271#file1008271line72> > > > > This is a bit unclean. I think its more idiomatic when the catch block > > includes the System.exit(1). > > > > Also, I'm afraid that printing the entire stack trace is intimidating > > to non-developers who use the CLI. Perhaps the stack trace should go under > > log.error(...)? > > Manikumar Reddy O wrote: > Calling System.exit(1) in catch block results unexecuted finally block. > > > http://stackoverflow.com/questions/1410951/how-does-javas-system-exit-work-with-try-catch-finally-blocks > > http://stackoverflow.com/questions/15264076/regarding-excuting-finally-block-in-system-exit-case-also-by-adding-shutdownhook?lq=1 > > log.error() used for printing stackTrace.
Good point. The problem is that the code here is very explicit about what we do when an exception occured, but doesn't show what we do when an exception doesn't occure. Putting "if ... else ... " at this point duplicates the "try ... catch... " logic right above it. How about modifying these lines to "System.exit(exitCode)" in the finally clause and setting the value of "exitCode" in the try and catch clauses? This will also allow us to support multiple exit codes cleanly in the future. - Gwen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34403/#review91310 ----------------------------------------------------------- On July 10, 2015, 5:44 p.m., Manikumar Reddy O wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34403/ > ----------------------------------------------------------- > > (Updated July 10, 2015, 5:44 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2198 > https://issues.apache.org/jira/browse/KAFKA-2198 > > > Repository: kafka > > > Description > ------- > > Addressing Gwen's comments > > > Diffs > ----- > > core/src/main/scala/kafka/admin/TopicCommand.scala > a2ecb9620d647bf8f957a1f00f52896438e804a7 > > Diff: https://reviews.apache.org/r/34403/diff/ > > > Testing > ------- > > > Thanks, > > Manikumar Reddy O > >
