ctubbsii commented on code in PR #3620:
URL: https://github.com/apache/accumulo/pull/3620#discussion_r1276745334
##########
server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooPropEditor.java:
##########
@@ -118,9 +126,27 @@ private void setProperty(final ServerContext context,
final PropStoreKey<?> prop
throw new IllegalArgumentException(
"Invalid set property format. Requires name=value, received " +
opts.setOpt);
}
- String[] tokens = opts.setOpt.split("=");
- Map<String,String> propValue = Map.of(tokens[0].trim(), tokens[1].trim());
- PropUtil.setProperties(context, propKey, propValue);
+ String targetName = "'invalid'";
+ try {
+ targetName = getDisplayName(propKey, context.getInstanceID(),
context.getZooReader());
+
+ Map<String,String> prev = context.getPropStore().get(propKey).asMap();
+ String[] tokens = opts.setOpt.split("=");
+ String propName = tokens[0].trim();
+ String propVal = tokens[1].trim();
+ Map<String,String> propMap = Map.of(propName, propVal);
+ PropUtil.setProperties(context, propKey, propMap);
+
+ if (prev.containsKey(propName)) {
+ LOG.info("{}: modified {} from {} to {}", targetName, propName,
prev.get(propName),
+ propVal);
+ } else {
+ LOG.info("{}: set {}={}", targetName, propName, propVal);
+ }
+ } catch (Exception ex) {
+ throw new IllegalStateException(
+ "Failed to set property for " + targetName + " (id: " +
propKey.getId() + ")", ex);
Review Comment:
Remapping all Exceptions to IllegalStateException seems weird. Why do this?
Can we narrow the catch a bit?
##########
server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooPropEditor.java:
##########
@@ -107,6 +111,10 @@ public void execute(String[] args) throws Exception {
default:
throw new IllegalArgumentException("Invalid operation requested");
}
+ } catch (Exception ex) {
+ LOG.error("{} command failed", keyword(), ex);
+ // hard fail - set exit status
+ System.exit(-1);
Review Comment:
Negative exit codes are not standard. The numbers should be 0 for success,
and positive, non-zero for failure. The negation of `0` in binary is `1`, not
`-1`. I know we have some existing code that uses negative one, but those
should be fixed as well. Eventually all of these should be fixed. There's some
discussion about this at
https://stackoverflow.com/questions/47265667/return-code-on-failure-positive-or-negative
In particular, if you try to do this in Java, the process exit code is 255,
not `-1`. Consider:
```java
public class Test {
public static void main(String[] args) {
System.exit(Integer.parseInt(args[0]));
}
}
```
Then run it:
```bash
$ for x in {-2..2}; do java Test.java $x; echo $?; done
254
255
0
1
2
```
This means that when you use negative exit codes in Java, you get a positive
exit code that is not the same value that you told it to use.
Also, I'm not sure why we're explicitly calling `System.exit` at all. The
exception should just fall through the `execute` method into whatever `main`
method called it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
