ctubbsii commented on code in PR #2120:
URL: https://github.com/apache/zookeeper/pull/2120#discussion_r1755356759
##########
bin/zkEnv.sh:
##########
@@ -145,9 +145,15 @@ fi
#echo "CLASSPATH=$CLASSPATH"
# default heap for zookeeper server
-ZK_SERVER_HEAP="${ZK_SERVER_HEAP:-1000}"
-export SERVER_JVMFLAGS="-Xmx${ZK_SERVER_HEAP}m $SERVER_JVMFLAGS"
+if [[ -n $ZK_SERVER_HEAP ]]; then
+ echo 'WARNING: ZK_SERVER_HEAP will not be recognized in a future version.
Set -Xmx directly using SERVER_JVMFLAGS instead.'
+ SERVER_JVMFLAGS="-Xmx${ZK_SERVER_HEAP}m $SERVER_JVMFLAGS"
+fi
+export SERVER_JVMFLAGS=${SERVER_JVMFLAGS:--Xmx1000m}
Review Comment:
A small modification could be made to avoid the "breakage" of not having a
default when the `SERVER_JVMFLAGS` is altered to set something other than the
max heap size, and the deprecated property isn't used:
```bash
if [[ -n $ZK_SERVER_HEAP ]]; then
echo 'WARNING: ZK_SERVER_HEAP will not be recognized in a future
version. Set -Xmx directly using SERVER_JVMFLAGS instead.'
SERVER_JVMFLAGS="-Xmx${ZK_SERVER_HEAP}m $SERVER_JVMFLAGS"
else
SERVER_JVMFLAGS="-Xmx1000m $SERVER_JVMFLAGS" # this ensures a default
value is always prepended
fi
export SERVER_JVMFLAGS
```
This works if the user wants to override with a different `-Xmx` value (the
latter will be used). However, that leaves the situation in the current state
of not allowing the `-Xmx` to be overridden with a related, but incompatible
option, like `-XX:MaxRAMPercentage`, which was the whole point of this effort
to add (or to avoid) an additional environmental property to handle it. This
implementation was suggested and pursued, because it was minimally complex, and
put more control in users hands. The other alternatives considered were
creating new properties and conditional logic for other memory related options
(originally suggested in ZOOKEEPER-4797), or (as @kezhuw suggested)
conditionally prepending, depending on whether one of the related options was
detected. I think both of these are overly complex, and give the appearance of
supporting more use cases, while actually supporting the same number of use
cases as the simpler solution, but with extra complexity the user needs to na
vigate in order to set the options correctly for their situation.
That is why the following is a better fix for ZOOKEEPER-1670:
```bash
export SERVER_JVMFLAGS=${SERVER_JVMFLAGS:--Xmx1000m}
```
If you want to keep this simple, minimally complex, but maximally
configurable, solution, but still want to alert users to the change in
behavior, because you're concerned it will break people who override
`SERVER_JVMFLAGS` for reasons unrelated to memory, then you can do something
like this to add the extra notification:
```bash
if [[ -n $ZK_SERVER_HEAP ]]; then
echo "INFO: SERVER_JVMFLAGS set by user ($SERVER_JVMFLAGS) will override
the default value (-Xmx1000m)"
fi
if [[ -n $ZK_SERVER_HEAP ]]; then
echo 'WARNING: ZK_SERVER_HEAP will not be recognized in a future
version. Set -Xmx directly using SERVER_JVMFLAGS instead.'
SERVER_JVMFLAGS="-Xmx${ZK_SERVER_HEAP}m $SERVER_JVMFLAGS"
fi
export SERVER_JVMFLAGS=${SERVER_JVMFLAGS:--Xmx1000m}
```
--
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...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
