accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-3604) connector.instanceOperations().setSystemProperty(String,String) returns silently for non-changeable ZK Property's
Date Wed, 29 Apr 2015 00:37:06 GMT

    [ https://issues.apache.org/jira/browse/ACCUMULO-3604?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14518449#comment-14518449
] 

Josh Elser commented on ACCUMULO-3604:
--------------------------------------

{code}
+  public static boolean setSystemProperty(String property, String value) throws KeeperException,
InterruptedException, IllegalArgumentException {
{code}

IllegalArgumentException is a RuntimeException, we don't need to explicitly declare this exception
via throws. Please remove it.

{code}
+    } catch (IllegalArgumentException iae) {
+      throw iae;
{code}

Same here; don't need to catch and rethrow. It'll happen naturally.

{code}
-    if ((p != null && !p.getType().isValidFormat(value)) || !Property.isValidZooPropertyKey(property))
{
+    if ((p != null && !p.getType().isValidFormat(value))) {
       log.warn("Ignoring property {} it is null, an invalid format, or not capable of being
changed in zookeeper", property);
       return false;
     }
{code}

Might it be good to also error out when the format isn't valid? It seems like this should
error just the same as a non-modifiable ZK property.

One thing to also consider if that the implementation of {{InstanceOperationImpl#setProperty(...)}}
invokes a method in the Master via Thrift. The client won't see an IllegalArgumentException,
but instead a TException. It would be better to either throw a named exception or return a
boolean for success/failure so the client can actually determine what happened on the server.
We should strive to be able to differentiate "success", "failure due to RPC arguments" and
"failure due to other reasons". Thoughts? Does that make sense?

> connector.instanceOperations().setSystemProperty(String,String) returns silently for
non-changeable ZK Property's
> -----------------------------------------------------------------------------------------------------------------
>
>                 Key: ACCUMULO-3604
>                 URL: https://issues.apache.org/jira/browse/ACCUMULO-3604
>             Project: Accumulo
>          Issue Type: Bug
>          Components: master
>    Affects Versions: 1.5.0, 1.5.1, 1.5.2, 1.6.0, 1.6.1, 1.6.2
>            Reporter: Josh Elser
>            Assignee: Jeffrey S Schwartz
>              Labels: newbie, summit2015
>             Fix For: 1.5.3, 1.6.3, 1.8.0, 1.7.1
>
>         Attachments: ACCUMULO-3604.patch
>
>
> Only a subset of the configuration {{Property}}'s in Accumulo are modifiable via ZooKeeper
(defined by {{Property.isValidZooProperty}}).
> {{connector.instanceOperations().setProperty(String,String)}} updates the provided property
name with the given value in ZooKeeper. The thing that is never mentioned is that only a subset
of the properties in Accumulo are allowed to be overriden in ZooKeeper. Furthermore, the user
receives no indication that their call failed.
> The Javadoc on {{setSystemProperty(String,String)}} should be updated to inform the users
that only some properties can be changed by this method, and some information should be returned
back to the user to let them know that their call did not succeed (likely an Exception).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message