You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/07/15 00:52:49 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3620: add feedback to zoo-prop-editor
EdColeman opened a new pull request, #3620:
URL: https://github.com/apache/accumulo/pull/3620
Updated zoo-prop-editor to provide feedback using log statements and improved exception messages as mentioned in PR #3445
- adds log statements showing success on set
- improved exception messages.
- command exit with -1 on errors.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3620: add feedback to zoo-prop-editor
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3620:
URL: https://github.com/apache/accumulo/pull/3620#discussion_r1281599914
##########
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:
I don't know. Given the fact that the exception would immediately fall out from a command the user initiated, I think most of these kinds of errors are probably informative enough, and it's just cleaner to let them continue to fall through.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3620: add feedback to zoo-prop-editor
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3620:
URL: https://github.com/apache/accumulo/pull/3620#discussion_r1276830744
##########
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:
The point of calling system exit was so that there was an explicit error return status - if some one were to write a script, I wanted to make sure that had an option to test the command return. I was not sure that throwing an exception would do that - and I wanted it to be explicit.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman merged pull request #3620: add feedback to zoo-prop-editor
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman merged PR #3620:
URL: https://github.com/apache/accumulo/pull/3620
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3620: add feedback to zoo-prop-editor
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3620:
URL: https://github.com/apache/accumulo/pull/3620#discussion_r1276830283
##########
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:
The called methods are throwing Runtime exceptions (Illegal Argument,...) The goal was to include what failed in the exception message to help troubleshoot what was being changed in the message. It could be done with a log message and a re-throw, but catching any exception, wrapping with the IllegalStateException with the augmented exception message seemed the most comprehensive and likely to provide consistent feedback.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3620: add feedback to zoo-prop-editor
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org