> 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. > > Gwen Shapira wrote: > 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.
Thank for the suggestion. Uploaded a new patch with relevant chnages. - Manikumar Reddy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34403/#review91310 ----------------------------------------------------------- On July 13, 2015, 1:57 p.m., Manikumar Reddy O wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34403/ > ----------------------------------------------------------- > > (Updated July 13, 2015, 1:57 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 > >
