You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Abraham Fine <ab...@abrahamfine.com> on 2016/06/22 22:27:07 UTC
Review Request 49112: ZOOKEEPER-1898
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49112/
-----------------------------------------------------------
Review request for zookeeper.
Bugs: ZOOKEEPER-1898
https://issues.apache.org/jira/browse/ZOOKEEPER-1898
Repository: zookeeper-git
Description
-------
zookeeper-cli always return "0" as exit code
Diffs
-----
src/java/main/org/apache/zookeeper/ZooKeeperMain.java f722693
src/java/main/org/apache/zookeeper/cli/CliCommand.java 527a19e
src/java/main/org/apache/zookeeper/cli/CreateCommand.java cc96939
src/java/main/org/apache/zookeeper/cli/DeleteCommand.java 4714be5
src/java/main/org/apache/zookeeper/cli/InvalidCommandException.java PRE-CREATION
src/java/main/org/apache/zookeeper/cli/ListQuotaCommand.java 907c466
src/java/main/org/apache/zookeeper/cli/LsCommand.java 43099b8
src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java f63cf0f
src/java/main/org/apache/zookeeper/cli/SetAclCommand.java f27446c
src/java/main/org/apache/zookeeper/cli/SetCommand.java 1c39377
src/java/main/org/apache/zookeeper/cli/SetQuotaCommand.java 873c6d2
src/java/test/org/apache/zookeeper/ZooKeeperTest.java 574eab0
Diff: https://reviews.apache.org/r/49112/diff/
Testing
-------
Thanks,
Abraham Fine
Re: Review Request 49112: ZOOKEEPER-1898
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49112/#review139890
-----------------------------------------------------------
Ship it!
lgtm.
- Patrick Hunt
On June 23, 2016, 10:17 p.m., Abraham Fine wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49112/
> -----------------------------------------------------------
>
> (Updated June 23, 2016, 10:17 p.m.)
>
>
> Review request for zookeeper.
>
>
> Bugs: ZOOKEEPER-1898
> https://issues.apache.org/jira/browse/ZOOKEEPER-1898
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> zookeeper-cli always return "0" as exit code
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ZooKeeperMain.java f722693
> src/java/main/org/apache/zookeeper/cli/AddAuthCommand.java e488745
> src/java/main/org/apache/zookeeper/cli/CliCommand.java 527a19e
> src/java/main/org/apache/zookeeper/cli/CliException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/CliParseException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/CliWrapperException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/CloseCommand.java 1d96639
> src/java/main/org/apache/zookeeper/cli/CommandNotFoundException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/CreateCommand.java cc96939
> src/java/main/org/apache/zookeeper/cli/DelQuotaCommand.java 2043f47
> src/java/main/org/apache/zookeeper/cli/DeleteAllCommand.java badf5b7
> src/java/main/org/apache/zookeeper/cli/DeleteCommand.java 4714be5
> src/java/main/org/apache/zookeeper/cli/GetAclCommand.java e646a04
> src/java/main/org/apache/zookeeper/cli/GetCommand.java 0ec8270
> src/java/main/org/apache/zookeeper/cli/GetConfigCommand.java 8c6a4f5
> src/java/main/org/apache/zookeeper/cli/ListQuotaCommand.java 907c466
> src/java/main/org/apache/zookeeper/cli/Ls2Command.java 2b02583
> src/java/main/org/apache/zookeeper/cli/LsCommand.java 43099b8
> src/java/main/org/apache/zookeeper/cli/MalformedCommandException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
> src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java f63cf0f
> src/java/main/org/apache/zookeeper/cli/SetAclCommand.java f27446c
> src/java/main/org/apache/zookeeper/cli/SetCommand.java 1c39377
> src/java/main/org/apache/zookeeper/cli/SetQuotaCommand.java 873c6d2
> src/java/main/org/apache/zookeeper/cli/StatCommand.java e5cfe54
> src/java/main/org/apache/zookeeper/cli/SyncCommand.java c12d35d
> src/java/test/org/apache/zookeeper/ZooKeeperTest.java 574eab0
>
> Diff: https://reviews.apache.org/r/49112/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Fine
>
>
Re: Review Request 49112: ZOOKEEPER-1898
Posted by Abraham Fine <ab...@abrahamfine.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49112/
-----------------------------------------------------------
(Updated June 23, 2016, 10:17 p.m.)
Review request for zookeeper.
Bugs: ZOOKEEPER-1898
https://issues.apache.org/jira/browse/ZOOKEEPER-1898
Repository: zookeeper-git
Description
-------
zookeeper-cli always return "0" as exit code
Diffs (updated)
-----
src/java/main/org/apache/zookeeper/ZooKeeperMain.java f722693
src/java/main/org/apache/zookeeper/cli/AddAuthCommand.java e488745
src/java/main/org/apache/zookeeper/cli/CliCommand.java 527a19e
src/java/main/org/apache/zookeeper/cli/CliException.java PRE-CREATION
src/java/main/org/apache/zookeeper/cli/CliParseException.java PRE-CREATION
src/java/main/org/apache/zookeeper/cli/CliWrapperException.java PRE-CREATION
src/java/main/org/apache/zookeeper/cli/CloseCommand.java 1d96639
src/java/main/org/apache/zookeeper/cli/CommandNotFoundException.java PRE-CREATION
src/java/main/org/apache/zookeeper/cli/CreateCommand.java cc96939
src/java/main/org/apache/zookeeper/cli/DelQuotaCommand.java 2043f47
src/java/main/org/apache/zookeeper/cli/DeleteAllCommand.java badf5b7
src/java/main/org/apache/zookeeper/cli/DeleteCommand.java 4714be5
src/java/main/org/apache/zookeeper/cli/GetAclCommand.java e646a04
src/java/main/org/apache/zookeeper/cli/GetCommand.java 0ec8270
src/java/main/org/apache/zookeeper/cli/GetConfigCommand.java 8c6a4f5
src/java/main/org/apache/zookeeper/cli/ListQuotaCommand.java 907c466
src/java/main/org/apache/zookeeper/cli/Ls2Command.java 2b02583
src/java/main/org/apache/zookeeper/cli/LsCommand.java 43099b8
src/java/main/org/apache/zookeeper/cli/MalformedCommandException.java PRE-CREATION
src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java f63cf0f
src/java/main/org/apache/zookeeper/cli/SetAclCommand.java f27446c
src/java/main/org/apache/zookeeper/cli/SetCommand.java 1c39377
src/java/main/org/apache/zookeeper/cli/SetQuotaCommand.java 873c6d2
src/java/main/org/apache/zookeeper/cli/StatCommand.java e5cfe54
src/java/main/org/apache/zookeeper/cli/SyncCommand.java c12d35d
src/java/test/org/apache/zookeeper/ZooKeeperTest.java 574eab0
Diff: https://reviews.apache.org/r/49112/diff/
Testing
-------
Thanks,
Abraham Fine
Re: Review Request 49112: ZOOKEEPER-1898
Posted by Patrick Hunt <ph...@apache.org>.
> On June 22, 2016, 10:52 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ZooKeeperMain.java, line 626
> > <https://reviews.apache.org/r/49112/diff/1/?file=1427805#file1427805line626>
> >
> > See my comment on the exception class. Should we have more than one exit code, ie based on the type of exception. I'm not sure if that's valuable, but it seems like it might be for someone trying to script this?
>
> Abraham Fine wrote:
> That is something that may be worth doing but I thought that the work of determining what that mapping would be (which may involve changing which exceptions are thrown) might have been outside the scope of this jira. This patch makes it very easy to add this kind of mapping in the future. Do you think this is something that I should change in this patch?
I think that's fine.
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49112/#review139157
-----------------------------------------------------------
On June 22, 2016, 10:27 p.m., Abraham Fine wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49112/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 10:27 p.m.)
>
>
> Review request for zookeeper.
>
>
> Bugs: ZOOKEEPER-1898
> https://issues.apache.org/jira/browse/ZOOKEEPER-1898
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> zookeeper-cli always return "0" as exit code
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ZooKeeperMain.java f722693
> src/java/main/org/apache/zookeeper/cli/CliCommand.java 527a19e
> src/java/main/org/apache/zookeeper/cli/CreateCommand.java cc96939
> src/java/main/org/apache/zookeeper/cli/DeleteCommand.java 4714be5
> src/java/main/org/apache/zookeeper/cli/InvalidCommandException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/ListQuotaCommand.java 907c466
> src/java/main/org/apache/zookeeper/cli/LsCommand.java 43099b8
> src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
> src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java f63cf0f
> src/java/main/org/apache/zookeeper/cli/SetAclCommand.java f27446c
> src/java/main/org/apache/zookeeper/cli/SetCommand.java 1c39377
> src/java/main/org/apache/zookeeper/cli/SetQuotaCommand.java 873c6d2
> src/java/test/org/apache/zookeeper/ZooKeeperTest.java 574eab0
>
> Diff: https://reviews.apache.org/r/49112/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Fine
>
>
Re: Review Request 49112: ZOOKEEPER-1898
Posted by Abraham Fine <ab...@abrahamfine.com>.
> On June 22, 2016, 10:52 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/ZooKeeperMain.java, line 626
> > <https://reviews.apache.org/r/49112/diff/1/?file=1427805#file1427805line626>
> >
> > See my comment on the exception class. Should we have more than one exit code, ie based on the type of exception. I'm not sure if that's valuable, but it seems like it might be for someone trying to script this?
That is something that may be worth doing but I thought that the work of determining what that mapping would be (which may involve changing which exceptions are thrown) might have been outside the scope of this jira. This patch makes it very easy to add this kind of mapping in the future. Do you think this is something that I should change in this patch?
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49112/#review139157
-----------------------------------------------------------
On June 22, 2016, 10:27 p.m., Abraham Fine wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49112/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 10:27 p.m.)
>
>
> Review request for zookeeper.
>
>
> Bugs: ZOOKEEPER-1898
> https://issues.apache.org/jira/browse/ZOOKEEPER-1898
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> zookeeper-cli always return "0" as exit code
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ZooKeeperMain.java f722693
> src/java/main/org/apache/zookeeper/cli/CliCommand.java 527a19e
> src/java/main/org/apache/zookeeper/cli/CreateCommand.java cc96939
> src/java/main/org/apache/zookeeper/cli/DeleteCommand.java 4714be5
> src/java/main/org/apache/zookeeper/cli/InvalidCommandException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/ListQuotaCommand.java 907c466
> src/java/main/org/apache/zookeeper/cli/LsCommand.java 43099b8
> src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
> src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java f63cf0f
> src/java/main/org/apache/zookeeper/cli/SetAclCommand.java f27446c
> src/java/main/org/apache/zookeeper/cli/SetCommand.java 1c39377
> src/java/main/org/apache/zookeeper/cli/SetQuotaCommand.java 873c6d2
> src/java/test/org/apache/zookeeper/ZooKeeperTest.java 574eab0
>
> Diff: https://reviews.apache.org/r/49112/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Fine
>
>
Re: Review Request 49112: ZOOKEEPER-1898
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49112/#review139157
-----------------------------------------------------------
Looks pretty reasonable to me, however I do have a few high level questions on how far we might/should go. Thoughts?
Note that the patch doesn't apply to 3.4. Not sure if it's worth fixing there?
src/java/main/org/apache/zookeeper/ZooKeeperMain.java (line 626)
<https://reviews.apache.org/r/49112/#comment204310>
See my comment on the exception class. Should we have more than one exit code, ie based on the type of exception. I'm not sure if that's valuable, but it seems like it might be for someone trying to script this?
src/java/main/org/apache/zookeeper/cli/InvalidCommandException.java (line 20)
<https://reviews.apache.org/r/49112/#comment204308>
This seems very specific. Perhaps we can have a general base cli exception and this would be a subclass of that? The base exception could also provide the exit code, rather than hardcoding it as we have now....
Not sure if this would work but we could also have a subclass that understands keeper exceptions?
- Patrick Hunt
On June 22, 2016, 10:27 p.m., Abraham Fine wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49112/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 10:27 p.m.)
>
>
> Review request for zookeeper.
>
>
> Bugs: ZOOKEEPER-1898
> https://issues.apache.org/jira/browse/ZOOKEEPER-1898
>
>
> Repository: zookeeper-git
>
>
> Description
> -------
>
> zookeeper-cli always return "0" as exit code
>
>
> Diffs
> -----
>
> src/java/main/org/apache/zookeeper/ZooKeeperMain.java f722693
> src/java/main/org/apache/zookeeper/cli/CliCommand.java 527a19e
> src/java/main/org/apache/zookeeper/cli/CreateCommand.java cc96939
> src/java/main/org/apache/zookeeper/cli/DeleteCommand.java 4714be5
> src/java/main/org/apache/zookeeper/cli/InvalidCommandException.java PRE-CREATION
> src/java/main/org/apache/zookeeper/cli/ListQuotaCommand.java 907c466
> src/java/main/org/apache/zookeeper/cli/LsCommand.java 43099b8
> src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java 282af55
> src/java/main/org/apache/zookeeper/cli/RemoveWatchesCommand.java f63cf0f
> src/java/main/org/apache/zookeeper/cli/SetAclCommand.java f27446c
> src/java/main/org/apache/zookeeper/cli/SetCommand.java 1c39377
> src/java/main/org/apache/zookeeper/cli/SetQuotaCommand.java 873c6d2
> src/java/test/org/apache/zookeeper/ZooKeeperTest.java 574eab0
>
> Diff: https://reviews.apache.org/r/49112/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Abraham Fine
>
>