keith-turner commented on code in PR #5599:
URL: https://github.com/apache/accumulo/pull/5599#discussion_r2124312958
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -65,8 +64,19 @@ 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();
+ String oldBindParameter = opts.getAddress();
+ String newBindParameter =
siteConfig.get(Property.RPC_PROCESS_BIND_ADDRESS);
Review Comment:
If they are both set to non default values it would be good to throw an
exception and let the user sort it out.
```java
// TODO maybe add constant for "0.0.0.0" to ServerOpts that could be use here
if(!oldBindParameter.equals("0.0.0.0") && !newBindParameter.equals("")){
throw new IllegalArgumentException("Specified bind address via '-a' and
'-o rpc.bind.addr=<address>'");
}
```
##########
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;
Review Comment:
Could label the outer for loop like `outer : for (String prefix :
validPrefixes)` and then would not need the valid boolean.
```suggestion
continue outer:
```
##########
server/base/src/main/java/org/apache/accumulo/server/AbstractServer.java:
##########
@@ -65,8 +64,19 @@ 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();
+ String oldBindParameter = opts.getAddress();
+ String newBindParameter =
siteConfig.get(Property.RPC_PROCESS_BIND_ADDRESS);
+ if (oldBindParameter == null && newBindParameter.equals("")) {
Review Comment:
Looking at the impl of ServerOpts.getAddress() it seems like it will never
return null, so `oldBindParameter` could not be null. It will either return
what the user set or `0.0.0.0`. Maybe should have constant for its default
value of `0.0.0.0` that this code could reference.
##########
server/base/src/main/java/org/apache/accumulo/server/ServerOpts.java:
##########
@@ -24,7 +24,8 @@
public class ServerOpts extends ConfigOpts {
- @Parameter(names = {"-a", "--address"}, description = "address to bind to")
+ @Parameter(names = {"-a", "--address"},
+ description = "address to bind to (deprecated - use rpc.bind.addr
instead)")
Review Comment:
```suggestion
description = "address to bind to (deprecated - use rpc.bind.addr with
-o option instead)")
```
--
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-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
