You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by afine <gi...@git.apache.org> on 2017/04/24 21:56:14 UTC
[GitHub] zookeeper pull request #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
GitHub user afine opened a pull request:
https://github.com/apache/zookeeper/pull/240
ZOOKEEPER-2757: Incorrect path crashes zkCli
This issue is caused by us relying on `IllegalArgumentException` in `PathUtils#validatePath`. `IllegalArgumentException` is an unchecked exception and we never catch it within each individual *Command.java, so it bubbles up and killed the CLI.
Given that throwing `IllegalArgumentException` is part of ZooKeeper's API, I believe that unfortunately we can not change this behavior at this time. This patch catches `IllegalArgumentException` and wraps it, so the CLI prints an error but does not quit. I believe I handled all of the relevant commands, please check to make sure I am not missing one.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/afine/zookeeper ZOOKEEPER-2757
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/240.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #240
----
commit 46dce3f1c5c219b72e4e046c0bb42c1201d44238
Author: Abraham Fine <af...@apache.org>
Date: 2017-04-24T21:47:29Z
ZOOKEEPER-2757: Incorrect path crashes zkCli
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Posted by afine <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/240
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/240
@afine looks like one test (org.apache.zookeeper.ZooKeeperTest.testCreateNodeWithoutData) is broken because of this patch. please check upstream build, should be pretty trivial to fix. Wondering why the pre-commit build does not catch this test.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Posted by hanm <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Posted by afine <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #240: ZOOKEEPER-2757: Incorrect path crashes zkCli
Posted by hanm <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---