You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2023/02/10 17:11:57 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #3192: Removed arguments from server processes
dlmarion opened a new pull request, #3192:
URL: https://github.com/apache/accumulo/pull/3192
Removed options from ServerOpts and removed ServerOpts subclasses. Replaced these options with new Property definitions which can be passed to the server processes using the existing -o argument
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103082208
##########
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:
Yes, but that would enable someone to reintroduce a subclass of ConfigOpts for a server process at a later time.
--
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] dlmarion merged pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #3192:
URL: https://github.com/apache/accumulo/pull/3192
--
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 #3192: Removed arguments from server processes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103150226
##########
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
+
main "$@"
Review Comment:
so you don't think I should make *any* changes to `bin/accumulo` ?
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103203398
##########
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:
removed in fae4135
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103154696
##########
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:
all changes have been removed from accumulo-env.sh
--
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 #3192: Removed arguments from server processes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103106608
##########
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
+
main "$@"
Review Comment:
Gotcha. In any case, I don't think we should be hard-coding these options into `bin/accumulo` or creating env variables as proxies for them at all.
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103080704
##########
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:
No, we don't want to specify the port. Each server process has a port property already and some have the ability to do port search.
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103076712
##########
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
+
main "$@"
Review Comment:
I meant to put "$args" here...
--
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 #3192: Removed arguments from server processes
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103174553
##########
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:
If we need to re-add it in future, that's fine. If not, it stays deleted. It's internal code organization only, not public API, so it's okay to delete if we don't need 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
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103155060
##########
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
+
main "$@"
Review Comment:
all changes have been removed from bin/accumulo
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103149557
##########
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:
resolved in later commit
--
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] dlmarion commented on a diff in pull request #3192: Removed arguments from server processes
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3192:
URL: https://github.com/apache/accumulo/pull/3192#discussion_r1103148729
##########
assemble/bin/accumulo:
##########
@@ -89,4 +89,21 @@ function main() {
exec "${JAVA[@]}" "${JAVA_OPTS[@]}" org.apache.accumulo.start.Main "$@"
}
+args=("$@")
Review Comment:
should be resolved now
--
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