[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/240 @hanm I don't think it is ever safe to make assumptions about the state of the process after a bug/"programming problem" has been encountered. So I think its better to exit the process. I fixed the compiler warning. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/240 Agree with what you said, Abe, in a generic context : ) - for CLI use cases though I am wondering why we should crash even in cases as you mentioned "So if it does pop up for reasons other than a bad path" - in such case should we always print the error message and not crashing the CLI? Would that be more helpful for user experience comparing to crash and leave no ideas to users what went wrong? Basically my point is that I don't any use case where the CLI's state is so bad that it can't recover from an unchecked exception so it has to be killed. Other than this patch looks good to me. One nit - a new compiler waring is introduced. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/240
@hanm I'm not sure that we should always assume that an
IllegalArgumentException will only be thrown when a path is invalid. Since
IllegalArgumentException does not need to be caught, it will be difficult for
us to tell if we are masking IllegalArgumentExceptions thrown for other
reasons. So we should minimize the code surrounded by `try { }
catch(IllegalArgumentException e) {}` We cannot catch them within the methods
of ZooKeeper.java so I think catching them at the "Command" level is the best
we can do.
IllegalArgumentException is a RuntimeException, which should represent a
"programming problem"
(http://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html).
>Runtime exceptions represent problems that are the result of a programming
problem, and as such, the API client code cannot reasonably be expected to
recover from them or to handle them in any way.
So if it does pop up for reasons other than a bad path, we should certainly
bubble it up and kill the CLI.
We abuse IllegalArgumentException all over ZK, I believe we should change
this exception type in 3.6.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Github user hanm commented on the issue: https://github.com/apache/zookeeper/pull/240 How about catch the IllegalArgumentException at ZooKeeperMain.processCmd and directly print the exception message? This way we don't need to make change to individual command. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
