Patrick Hunt updated ZOOKEEPER-217:

    Status: Open  (was: Patch Available)

I see a few issues that should be addressed:

1) we should use LOG.fatal instead of LOG.error when about to exit (in main for 

2) the following exception does not indicate the config param in question -- it 
might be hard to track down what has to be changed in this case, in the other 
exceptions you indicate the particular field/variable/etc... that needs to be 

+                throw new IllegalArgumentException(myIdString
+                        + " is not a number");

3) why is parse changed to "throws Exception" rather than the more explicit 
"throws IllegalArgumentException"?

4) specifying the usage in parse seems wrong to me:

+        if (args.length != 1) {
+            throw new IllegalArgumentException("USAGE: configFile");
+        }

How do you know what the usage is going to be? That's up to the main routine. 
This will result in confusion for the user.

+            throw new IllegalArgumentException("USAGE: ZooKeeperServer port dat

> Errors in config file
> ---------------------
>                 Key: ZOOKEEPER-217
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-217
>             Project: Zookeeper
>          Issue Type: Improvement
>          Components: server
>    Affects Versions: 3.0.0
>            Reporter: Flavio Paiva Junqueira
>            Assignee: Mahadev konar
>            Priority: Minor
>             Fix For: 3.0.1, 3.1.0
>         Attachments: ZOOKEEPER-217.patch
> Discussing 209 with Ben today, we thought that it would be better to have the 
> parse method of QuorumPeerConfig returning a boolean that indicates whether 
> the configuration is good or not, and let the caller decide whether to exit 
> or not. Currently we execute a System.exit() on QuorumPeerConfig.parse when 
> we have a critical configuration error.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to