ctubbsii commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103061714
##########
assemble/bin/accumulo:
##########
@@ -89,4 +89,21 @@ function main() {
exec "${JAVA[@]}" "${JAVA_OPTS[@]}" org.apache.accumulo.start.Main "$@"
}
+args=("$@")
Review Comment:
I'm not sure what this is doing. It doesn't look like this assignment is
used.
##########
assemble/bin/accumulo:
##########
@@ -89,4 +89,21 @@ function main() {
exec "${JAVA[@]}" "${JAVA_OPTS[@]}" org.apache.accumulo.start.Main "$@"
}
+args=("$@")
+
+if [ -n "$ACCUMULO_BIND_ADDR" ]; then
+ args+=("-o")
+ args+=("general.process.bind.addr=${env:ACCUMULO_BIND_ADDR}")
+fi
+
+if [ -n "$ACCUMULO_COMPACTOR_QUEUE" ]; then
+ args+=("-o")
+ args+=("compactor.queue=${env:ACCUMULO_COMPACTOR_QUEUE}")
+fi
+
+if [ -n "$ACCUMULO_SSERVER_GROUP" ]; then
+ args+=("-o")
+ args+=("sserver.group=${env:ACCUMULO_SSERVER_GROUP}")
+fi
Review Comment:
You can combine these into one line. Bash arrays are space delimited.
However, I don't think this will work in the bash script. The `${env:XXX}`
syntax works inside the configuration file. Here in bash, it should just be
`$XXX`.
Also, I think we've standardized on the double square braces `[[`, so we're
not potentially executing an external process, `/usr/bin/[` (yes, that's an
actual process and how POSIX shells process the contents of the square braces;
the double brace makes it clear that it's done inside bash as a language
feature, not as an external process).
```suggestion
if [[ -n "$ACCUMULO_BIND_ADDR" ]]; then
args+=("-o" "general.process.bind.addr=$ACCUMULO_BIND_ADDR")
fi
if [[ -n "$ACCUMULO_COMPACTOR_QUEUE" ]]; then
args+=("-o" "compactor.queue=$ACCUMULO_COMPACTOR_QUEUE")
fi
if [[ -n "$ACCUMULO_SSERVER_GROUP" ]; then
args+=("-o" "sserver.group=$ACCUMULO_SSERVER_GROUP")
fi
```
##########
server/base/src/main/java/org/apache/accumulo/server/ServerOpts.java:
##########
@@ -20,17 +20,11 @@
import org.apache.accumulo.core.cli.ConfigOpts;
-import com.beust.jcommander.Parameter;
+public final class ServerOpts extends ConfigOpts {
-public class ServerOpts extends ConfigOpts {
+ // This class is empty on purpose. The intent here is that
+ // the Accumulo server processes will only ConfigOpts. Can't
+ // make ConfigOpts final as it's used by utility classes
+ // that subclass it.
Review Comment:
Can we just delete this class?
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -284,6 +285,9 @@ public enum Property {
PropertyType.BOOLEAN, "Enables JVM metrics functionality using
Micrometer", "2.1.0"),
GENERAL_MICROMETER_FACTORY("general.micrometer.factory", "",
PropertyType.CLASSNAME,
"Name of class that implements MeterRegistryFactory", "2.1.0"),
+ GENERAL_PROCESS_BIND_ADDRESS("general.process.bind.addr", "0.0.0.0",
PropertyType.STRING,
+ "The local IP address to which this server should bind for sending and
receiving network traffic",
+ "3.0.0"),
Review Comment:
Should the bind address include the bind port, or do we still want to keep
the port option separate?
##########
assemble/conf/accumulo-env.sh:
##########
@@ -144,3 +144,6 @@ esac
## Specifies command that will be placed before calls to Java in accumulo
script
# export ACCUMULO_JAVA_PREFIX=""
+# export ACCUMULO_BIND_ADDR=""
+# export ACCUMULO_COMPACTOR_QUEUE=""
+# export ACCUMULO_SSERVER_GROUP=""
Review Comment:
We should not be adding new environment variables. One of the design goals
of the scripts starting in 2.0 was to be less tightly coupled to environment
variables baked in to our startup scripts. These lock users in to features of
`bin/accumulo` and makes `bin/accumulo` do more than merely "run java main
class with provided args".
Rather than add these environment variables, and then bake in the options
into the `bin/accumulo` script, users should just specify `-o <whatever>`
themselves, as needed. We do not need to, and should not, be selectively
picking a subset of our configuration options, and providing yet-another-way to
configure those same options. This creates confusion and makes it hard for
users to know the "right" or "best" way to configure things.
--
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]
