ctubbsii commented on code in PR #5599:
URL: https://github.com/apache/accumulo/pull/5599#discussion_r2130648638
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -65,8 +64,20 @@ protected AbstractServer(String appName, ServerOpts opts,
String[] args) {
this.log = LoggerFactory.getLogger(getClass().getName());
this.applicationName = appName;
opts.parseArgs(appName, args);
- this.hostname = Objects.requireNonNull(opts.getAddress());
var siteConfig = opts.getSiteConfiguration();
+ final String oldBindParameter = opts.getAddress();
+ if (!oldBindParameter.equals(ServerOpts.BIND_ALL_ADDRESSES)
+ && siteConfig.isPropertySet(Property.RPC_PROCESS_BIND_ADDRESS)) {
+ throw new IllegalStateException(
+ "Bind address specified via '-a' and '-o rpc.bind.addr=<address>'");
+ }
+ final String newBindParameter =
siteConfig.get(Property.RPC_PROCESS_BIND_ADDRESS);
+ if (oldBindParameter.equals(ServerOpts.BIND_ALL_ADDRESSES)
+ &&
!newBindParameter.equals(Property.RPC_PROCESS_BIND_ADDRESS.getDefaultValue())) {
+ this.hostname = newBindParameter;
+ } else {
+ this.hostname = oldBindParameter;
+ }
Review Comment:
This gets a little tricky, trying to understand user behavior. Let's say you
get here, and the user has specified `-a 0.0.0.0` on the command line, so
they've explicitly set it to the default value. But, they've also set
`rpc.bind.addr=a.b.c.d` in the `accumulo.properties` file. The user probably
believes they are overriding the address on the command-line, but this logic
will use `a.b.c.d`. It would be nice if we could detect whether the user has
explicitly set `-a` on the command-line, similar to how we have
`isPropertySet()` on the config. If the user has specified both options at
all... regardless of whether they've set them to the default values or not, we
should throw an exception. We should only use the old property if no attempt
was made at all to set the new property, and use the new property otherwise.
That was mimic the behavior of the utility we have for selecting between
replacement and deprecated properties, `AccumuloConfiguration.resolve()`.
Having the additional ex
ception if both are used is even better, but we should use the information
about whether the property/flag was used, not just check that the value it is
set to differs from the default value. Otherwise, we may not be interpreting
the user's intent correctly.
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
Review Comment:
We have no restrictions on property overrides. I believe you can override
any and all properties here... anything that you could set in the
SiteConfiguration (accumulo.properties file). That would include `instance.*`
properties as well. Setting instance properties this way might be rare in many
scenarios, because it's probably easier to ensure they are the same across a
cluster if they are set in the config file, rather than placed on the
command-line. However, in some scenarios (like passing args to a RUN directive
in a Dockerfile), it might be quite common to do that to customize a base image
at runtime.
My biggest concern here is that the usage will only print a subset of things
that the user can override, and even then, it can be quite a lot. So, it can be
very misleading that it's an exhaustive list, while also being a very noisy
output for something that most people will probably not even use. Most people
should probably prefer to put their configs in the config file only; others
might specify the config file explicitly using
`-Daccumulo.properties=file:///path/to/configFile`; I think it should be pretty
rare to use `-o`, used only in some particular cases. Ideally, users shouldn't
even see the usage ever... launching services should be scripted using
well-established launch scripts.
To have so much usage output, where it could be misunderstood to be
exhaustive when it isn't, and when users should probably not even be using `-o`
for most of these properties at all, anyway, seems like this extra usage is
unwarranted. I think it would be better to just have a brief usage that says
what the `-o` option does, and refer people to the documentation for the list
of properties.
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
+
+ final String propOnlyFormat =
+ "%1$-" + maxPropLength + "s %2$-" + Math.min(52, maxDefaultLength) +
"s";
+ final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
+ final String replacedFormat = propOnlyFormat + " (deprecated - replaced by
%3$s)";
Review Comment:
I think you should omit the reordering indexes in the format string, because
1) you're not actually using them to reorder the parameters, and 2) most people
don't use these, so these reduce the readability of the format string, which is
already a bit complex because of the dynamic resizing.
```suggestion
"%-" + maxPropLength + "s %-" + Math.min(52, maxDefaultLength) + "s";
final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
final String replacedFormat = propOnlyFormat + " (deprecated - replaced
by %s)";
```
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
Review Comment:
This is included with the general prefix, so it doesn't need to be specified
again.
```suggestion
```
##########
core/src/main/java/org/apache/accumulo/core/cli/Help.java:
##########
@@ -41,10 +41,18 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
if (help) {
commander.usage();
+ String extra = getAdditionalHelpInformation(programName);
+ if (extra != null) {
+ commander.getConsole().println(extra);
+ }
Review Comment:
One important limitation of this is that there are property prefixes that
represent user-configurable properties but which don't have pre-defined fixed
property names, because the property names themselves are dynamic. For example:
`TSERV_SCAN_EXECUTORS_PREFIX` or `TSERV_COMPACTION_SERVICE_PREFIX` or
`TSERV_WAL_SORT_FILE_PREFIX`
These changes completely skip those.
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
+
+ final String propOnlyFormat =
+ "%1$-" + maxPropLength + "s %2$-" + Math.min(52, maxDefaultLength) +
"s";
+ final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
+ final String replacedFormat = propOnlyFormat + " (deprecated - replaced by
%3$s)";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append(
+ "\tBelow are the properties, and their default values, that can be
used with the '-o' (overrides) option.\n");
+ sb.append("\tLong default values will be truncated.\n");
+ sb.append(
+ "\tSee the user guide at https://accumulo.apache.org/ for more
information about each property.\n");
+ sb.append("\n");
+
+ final SortedSet<Property> sortedProperties = new TreeSet<>(new
Comparator<Property>() {
+ @Override
+ public int compare(Property arg0, Property arg1) {
+ return arg0.getKey().compareTo(arg1.getKey());
+ }
+ });
+
+ for (Property p : Property.values()) {
+ sortedProperties.add(p);
+ }
+
+ for (Property prop : sortedProperties) {
+ if (prop.getType() == PropertyType.PREFIX) {
+ continue;
+ }
+ final String key = prop.getKey();
+ boolean valid = false;
+ for (String prefix : validPrefixes) {
+ if (key.startsWith(prefix)) {
+ valid = true;
+ break;
+ }
+ }
+ if (!valid) {
+ continue;
+ }
+ String value = prop.getDefaultValue();
+ if (value.length() > 40) {
+ value = value.substring(0, 40) + " (truncated)";
+ }
+ if (!prop.isDeprecated() && !prop.isReplaced()) {
+ sb.append(String.format(propOnlyFormat, key, value));
+ } else if (prop.isDeprecated() && !prop.isReplaced()) {
+ sb.append(String.format(deprecatedOnlyFormat, key, value));
+ } else {
+ sb.append(String.format(replacedFormat, key, value,
prop.replacedBy().getKey()));
+ }
+ sb.append("\n");
+ }
+ return sb.toString();
+ }
Review Comment:
I rewrote this more succinctly:
```suggestion
Stream.of(Property.values()).filter(p -> p.getType() !=
PropertyType.PREFIX)
.collect(
Collectors.toCollection(() -> new
TreeSet<>(Comparator.comparing(Property::getKey))))
.forEach(prop -> {
final String key = prop.getKey();
validPrefixes.stream().filter(key::startsWith).findFirst().ifPresent(prefix -> {
String value = prop.getDefaultValue();
if (value.length() > 40) {
value = value.substring(0, 40) + " (truncated)";
}
if (!prop.isDeprecated() && !prop.isReplaced()) {
sb.append(String.format(propOnlyFormat, key, value));
} else if (prop.isDeprecated() && !prop.isReplaced()) {
sb.append(String.format(deprecatedOnlyFormat, key, value));
} else {
sb.append(String.format(replacedFormat, key, value,
prop.replacedBy().getKey()));
}
sb.append("\n");
});
});
return sb.toString();
}
```
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
+
+ final String propOnlyFormat =
+ "%1$-" + maxPropLength + "s %2$-" + Math.min(52, maxDefaultLength) +
"s";
+ final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
+ final String replacedFormat = propOnlyFormat + " (deprecated - replaced by
%3$s)";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append(
+ "\tBelow are the properties, and their default values, that can be
used with the '-o' (overrides) option.\n");
+ sb.append("\tLong default values will be truncated.\n");
+ sb.append(
+ "\tSee the user guide at https://accumulo.apache.org/ for more
information about each property.\n");
+ sb.append("\n");
+
+ final SortedSet<Property> sortedProperties = new TreeSet<>(new
Comparator<Property>() {
+ @Override
+ public int compare(Property arg0, Property arg1) {
+ return arg0.getKey().compareTo(arg1.getKey());
+ }
+ });
+
+ for (Property p : Property.values()) {
+ sortedProperties.add(p);
+ }
+
+ for (Property prop : sortedProperties) {
+ if (prop.getType() == PropertyType.PREFIX) {
+ continue;
+ }
+ final String key = prop.getKey();
+ boolean valid = false;
+ for (String prefix : validPrefixes) {
+ if (key.startsWith(prefix)) {
+ valid = true;
+ break;
+ }
+ }
+ if (!valid) {
+ continue;
+ }
+ String value = prop.getDefaultValue();
+ if (value.length() > 40) {
+ value = value.substring(0, 40) + " (truncated)";
+ }
Review Comment:
It seems strange to truncate default values. If we have a default value that
long, it's probably okay to keep it? I'm not sure. I'd have to see the output
side-by-side.
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
+
+ final String propOnlyFormat =
+ "%1$-" + maxPropLength + "s %2$-" + Math.min(52, maxDefaultLength) +
"s";
+ final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
+ final String replacedFormat = propOnlyFormat + " (deprecated - replaced by
%3$s)";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append(
+ "\tBelow are the properties, and their default values, that can be
used with the '-o' (overrides) option.\n");
+ sb.append("\tLong default values will be truncated.\n");
+ sb.append(
+ "\tSee the user guide at https://accumulo.apache.org/ for more
information about each property.\n");
Review Comment:
Best to avoid tabs, and just used a fixed width spacing indentation, because
tab display depends too much on the environment.
##########
server/base/src/main/java/org/apache/accumulo/server/ServerOpts.java:
##########
@@ -24,13 +24,16 @@
public class ServerOpts extends ConfigOpts {
- @Parameter(names = {"-a", "--address"}, description = "address to bind to")
+ public static final String BIND_ALL_ADDRESSES = "0.0.0.0";
+
+ @Parameter(names = {"-a", "--address"},
+ description = "address to bind to (deprecated - use rpc.bind.addr with
-o option instead)")
Review Comment:
I think this might look better in the generated docs:
```suggestion
@Parameter(names = {"-a", "--address"},
description = "address to bind to (deprecated, use <code>-o
rpc.bind.addr=<address></code> instead)")
```
Or maybe this might work, since the docs are generated as markdown:
```suggestion
@Parameter(names = {"-a", "--address"},
description = "address to bind to (deprecated, use `-o
rpc.bind.addr=<address>` instead)")
```
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
+
+ final String propOnlyFormat =
+ "%1$-" + maxPropLength + "s %2$-" + Math.min(52, maxDefaultLength) +
"s";
+ final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
+ final String replacedFormat = propOnlyFormat + " (deprecated - replaced by
%3$s)";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append(
+ "\tBelow are the properties, and their default values, that can be
used with the '-o' (overrides) option.\n");
+ sb.append("\tLong default values will be truncated.\n");
+ sb.append(
Review Comment:
I am looking forward to Java 17 (https://openjdk.org/jeps/378)
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
Review Comment:
I know you're not a fan of streams, but integer streams make computing the
max value trivial without needing to evaluate the correctness of your code here:
```suggestion
int maxPropLength =
Stream.of(Property.values()).mapToInt(p ->
p.getKey().length()).max().orElse(0);
int maxDefaultLength =
Stream.of(Property.values()).map(Property::getDefaultValue)
.filter(Objects::nonNull).mapToInt(String::length).max().orElse(0);
```
##########
core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java:
##########
@@ -106,4 +112,113 @@ public void parseArgs(String programName, String[] args,
Object... others) {
}
}
}
+
+ @Override
+ public String getAdditionalHelpInformation(String programName) {
+
+ final Set<String> validPrefixes = new HashSet<>();
+
+ switch (programName) {
+ case "compactor":
+ validPrefixes.add(Property.COMPACTOR_PREFIX.getKey());
+ break;
+ case "compaction-coordinator":
+ validPrefixes.add(Property.COMPACTION_COORDINATOR_PREFIX.getKey());
+ break;
+ case "gc":
+ validPrefixes.add(Property.GC_PREFIX.getKey());
+ break;
+ case "manager":
+ validPrefixes.add(Property.MANAGER_PREFIX.getKey());
+ break;
+ case "monitor":
+ validPrefixes.add(Property.MONITOR_PREFIX.getKey());
+ break;
+ case "sserver":
+ validPrefixes.add(Property.SSERV_PREFIX.getKey());
+ break;
+ case "tserver":
+ validPrefixes.add(Property.TSERV_PREFIX.getKey());
+ break;
+ default:
+ break;
+ }
+
+ if (validPrefixes.isEmpty()) {
+ // We only provide extra help information for server processes
+ return null;
+ }
+
+ // print out possible property overrides for the -o argument.
+ validPrefixes.add(Property.GENERAL_PREFIX.getKey());
+ validPrefixes.add(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey());
+ validPrefixes.add(Property.RPC_PREFIX.getKey());
+
+ // Determine format lengths based on property names and default values
+ int maxPropLength = 0;
+ int maxDefaultLength = 0;
+ for (Property prop : Property.values()) {
+ if (prop.getKey().length() > maxPropLength) {
+ maxPropLength = prop.getKey().length();
+ }
+ if (prop.getDefaultValue() != null && prop.getDefaultValue().length() >
maxDefaultLength) {
+ maxDefaultLength = prop.getDefaultValue().length();
+ }
+ }
+
+ final String propOnlyFormat =
+ "%1$-" + maxPropLength + "s %2$-" + Math.min(52, maxDefaultLength) +
"s";
+ final String deprecatedOnlyFormat = propOnlyFormat + " (deprecated)";
+ final String replacedFormat = propOnlyFormat + " (deprecated - replaced by
%3$s)";
+
+ StringBuilder sb = new StringBuilder();
+ sb.append(
+ "\tBelow are the properties, and their default values, that can be
used with the '-o' (overrides) option.\n");
+ sb.append("\tLong default values will be truncated.\n");
+ sb.append(
+ "\tSee the user guide at https://accumulo.apache.org/ for more
information about each property.\n");
+ sb.append("\n");
+
+ final SortedSet<Property> sortedProperties = new TreeSet<>(new
Comparator<Property>() {
+ @Override
+ public int compare(Property arg0, Property arg1) {
+ return arg0.getKey().compareTo(arg1.getKey());
+ }
+ });
+
+ for (Property p : Property.values()) {
+ sortedProperties.add(p);
+ }
Review Comment:
```suggestion
final SortedSet<Property> sortedProperties =
new TreeSet<>(Comparator.comparing(Property::getKey));
sortedProperties.addAll(EnumSet.allOf(Property.class));
```
--
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]
