ctubbsii commented on code in PR #5966:
URL: https://github.com/apache/accumulo/pull/5966#discussion_r2558171089
##########
assemble/conf/accumulo-env.sh:
##########
@@ -159,3 +159,13 @@ esac
## environment, that will override what is set here, rather than some mangled
## merged result. You can set the variable any way you like.
#declare -p 'ACCUMULO_JAVA_PREFIX' &>/dev/null || ACCUMULO_JAVA_PREFIX=''
+
+## ACCUMULO_MAIN_ARGS can be used to pass extra arguments directly to
+## org.apache.accumulo.start.Main (after the JVM options). Declare as an array
+## to avoid issues with spaces in values.
+#case "$cmd" in
+# monitor) ACCUMULO_MAIN_ARGS=(-o "rpc.bind.port=9995") ;;
+# tserver) ACCUMULO_MAIN_ARGS=(-o "rpc.bind.port=20000-20049") ;;
+# compactor) ACCUMULO_MAIN_ARGS=(-o "rpc.bind.port=20050-20099") ;;
+# sserver) ACCUMULO_MAIN_ARGS=(-o "rpc.bind.port=20100-20149") ;;
Review Comment:
See my other comment for several better ways to do this. Also, the default
range should apply to all server types, without any need for this config at
all. I don't even think we need this as a commented-out example.
##########
core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java:
##########
@@ -423,43 +420,71 @@ public boolean test(final String input) {
}
- public static class PortRange extends Matches {
-
- public static final Range<Integer> VALID_RANGE = Range.of(1024, 65535);
-
- public PortRange(final String pattern) {
- super(pattern);
- }
+ private static class PortPredicate implements Predicate<String> {
@Override
public boolean test(final String input) {
- if (super.test(input)) {
+ if (input == null) {
+ return true;
+ }
+ final String trimmed = input.trim();
+ if (trimmed.isEmpty()) {
+ return false;
+ }
+ if ("0".equals(trimmed)) {
+ return true;
+ }
Review Comment:
Why is `0` valid here? It seems like it shouldn't be.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -395,8 +403,6 @@ was changed and it now can accept multiple class names. The
metrics spi was intr
// properties that are specific to manager server behavior
MANAGER_PREFIX("manager.", null, PropertyType.PREFIX,
"Properties in this category affect the behavior of the manager
server.", "2.1.0"),
- MANAGER_CLIENTPORT("manager.port.client", "9999", PropertyType.PORT,
- "The port used for handling client connections on the manager.",
"1.3.5"),
Review Comment:
It's probably a good idea to keep these around and deprecated. If the new
property is set, then we should use the new behavior and ignore anything in the
old properties. Otherwise, we should read the old properties and use them.
Ideally, we'd introduce this in 3.1 and deprecate them there, so we don't
need to have these in 4.0 at all.
Because this is a much simpler config, that hopefully users just don't have
to worry about much because the defaults are reasonable, I don't think it would
be completely terrible if we just changed this without a deprecation, but we
would definitely need to call that out in the release notes.
##########
core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java:
##########
@@ -423,43 +420,71 @@ public boolean test(final String input) {
}
- public static class PortRange extends Matches {
-
- public static final Range<Integer> VALID_RANGE = Range.of(1024, 65535);
-
- public PortRange(final String pattern) {
- super(pattern);
- }
+ private static class PortPredicate implements Predicate<String> {
@Override
public boolean test(final String input) {
- if (super.test(input)) {
+ if (input == null) {
+ return true;
+ }
+ final String trimmed = input.trim();
Review Comment:
If we're trimming for validation, we need to trim when we use it. It might
make sense here, but I think it would be novel. Looking at the code, we only
trim the input in PropertyType in two other places, and one might be a bug.
1. We trim when we strip the units off of the end of bounded units, so we
can isolate the numeric part, because we're only testing the number part.
2. We trim absolute paths during validation (might be a bug... seems sketchy
at a glance)
In general, we assume that the property value is exactly what the user
specified, including any spaces. I don't know that we should always do that,
but it would be nice if we were consistent, and it doesn't look like we do
anything to transform the user's input by stripping off whitespace in most of
the other property types, so we probably shouldn't here.
##########
assemble/bin/accumulo:
##########
@@ -86,7 +86,18 @@ function main() {
JAVA=("${ACCUMULO_JAVA_PREFIX[@]}" "$JAVA")
fi
- exec "${JAVA[@]}" "${JAVA_OPTS[@]}" org.apache.accumulo.start.Main "$@"
+ # Allow users to supply extra Accumulo arguments via ACCUMULO_MAIN_ARGS
+ if ! declare -p ACCUMULO_MAIN_ARGS &>/dev/null; then
+ ACCUMULO_MAIN_ARGS=()
+ else
+ declare_output=$(declare -p ACCUMULO_MAIN_ARGS 2>/dev/null)
+ if [[ $declare_output != declare\ -a* ]]; then
+ ACCUMULO_MAIN_ARGS_RAW=${ACCUMULO_MAIN_ARGS[*]}
+ read -r -a ACCUMULO_MAIN_ARGS <<<"${ACCUMULO_MAIN_ARGS_RAW}"
+ fi
+ fi
Review Comment:
So, I have a couple of thoughts on this:
1. This is making me really regret that I didn't push harder against the
`-o` property passing. My original proposal when we reworked the launch scripts
was to support Java system properties for accumulo configs, as in:
`-Daccumulo.rpc.bind.port=9999`. I don't know the reason Mike implemented the
`-o`, but it is really making it inconvenient here. If we could just pass them
as system properties, then this would be a non-issue. And, I still think that's
the better implementation.
2. The default value should be a range that servers of any type can share.
So, most of the time, users don't need to specify anything in their config
files.
3. If one is going to specify properties in config files, we don't need any
of this, because we can inject properties into the commons-configuration file
using property interpolation. If a user wants to set a custom port, they can
either use a shared `accumulo.properties` file that has an entry like
`rpc.bind.port=${env.RPC_PORT}` and then do `export RPC_PORT=9999` in
`accumulo-env.sh` based on the server type, or they can use something like
`rpc.bind.port=${sys:accumulo.rpc.bind.port}` and set
`-Daccumulo.rpc.bind.port=9999` in their `JAVA_OPTS` based on the server type,
OR they can import server-type-specific properties using
[includes](https://commons.apache.org/proper/commons-configuration/userguide/howto_properties.html#Includes)
that pull in a specific config for a specific server type, and they can spread
their config over multiple config files, OR they can choose different wholly
independent config files, by adding
`-Daccumulo.properties=/path/to/server-specific.properties
` in their `accumulo-env.sh` based on the server type.
So, we just don't need this. We already have better options than making the
scripts more complicated.
--
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]
