You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Manikumar Reddy O <ma...@gmail.com> on 2015/07/10 18:34:48 UTC
Re: Review Request 34403: Patch for KAFKA-2198
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34403/
-----------------------------------------------------------
(Updated July 10, 2015, 4:34 p.m.)
Review request for kafka.
Bugs: KAFKA-2198
https://issues.apache.org/jira/browse/KAFKA-2198
Repository: kafka
Description (updated)
-------
rebase
Diffs (updated)
-----
core/src/main/scala/kafka/admin/TopicCommand.scala a2ecb9620d647bf8f957a1f00f52896438e804a7
Diff: https://reviews.apache.org/r/34403/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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
>
>
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Manikumar Reddy O <ma...@gmail.com>.
> 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(...)?
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.
- Manikumar Reddy
-----------------------------------------------------------
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
>
>
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Gwen Shapira <gs...@cloudera.com>.
> 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
>
>
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34403/#review91310
-----------------------------------------------------------
Thanks for the patch!
core/src/main/scala/kafka/admin/TopicCommand.scala (lines 72 - 73)
<https://reviews.apache.org/r/34403/#comment144620>
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(...)?
- Gwen Shapira
On July 10, 2015, 4:34 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, 4:34 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2198
> https://issues.apache.org/jira/browse/KAFKA-2198
>
>
> Repository: kafka
>
>
> Description
> -------
>
> rebase
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/admin/TopicCommand.scala a2ecb9620d647bf8f957a1f00f52896438e804a7
>
> Diff: https://reviews.apache.org/r/34403/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Manikumar Reddy O
>
>
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34403/#review91579
-----------------------------------------------------------
Ship it!
Ship It!
- Gwen Shapira
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
>
>
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
core/src/main/scala/kafka/admin/TopicCommand.scala a2ecb9620d647bf8f957a1f00f52896438e804a7
Diff: https://reviews.apache.org/r/34403/diff/
Testing
-------
Thanks,
Manikumar Reddy O
Re: Review Request 34403: Patch for KAFKA-2198
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-------
Addressing Gwen's comments
Diffs (updated)
-----
core/src/main/scala/kafka/admin/TopicCommand.scala a2ecb9620d647bf8f957a1f00f52896438e804a7
Diff: https://reviews.apache.org/r/34403/diff/
Testing
-------
Thanks,
Manikumar Reddy O