bitflicker64 commented on code in PR #3253:
URL: https://github.com/apache/hugegraph/pull/3253#discussion_r4330742431
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -132,46 +132,64 @@ if [[ ! -r ${JVM_MODULE_OPTIONS} ]]; then
exit 1
fi
-# Set Java options
-if [ "$JAVA_OPTIONS" = "" ]; then
+# Set Java options.
+# Script-generated options are kept in arrays so that overridden paths
+# containing spaces (CONF/LOGS/PLUGINS) remain a single argument when passed to
+# java. User-supplied strings ($JAVA_OPTIONS / $USER_OPTION) keep the
historical
+# whitespace-splitting contract.
+JAVA_OPTIONS_ENV="${JAVA_OPTIONS}"
+JAVA_OPTIONS=()
+if [ "$JAVA_OPTIONS_ENV" = "" ]; then
XMX=$(calc_xmx $MIN_MEM $MAX_MEM)
if [ $? -ne 0 ]; then
report_error "Failed to start HugeGraphServer, requires at least
${MIN_MEM}MB free memory"
exit 1
fi
- JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m ${USER_OPTION}"
+ JAVA_OPTIONS=("-Xms${MIN_MEM}m" "-Xmx${XMX}m")
+
+ # Split user-supplied options on whitespace (legacy behaviour)
+ USER_OPTIONS=()
+ read -r -a USER_OPTIONS <<< "${USER_OPTION}"
Review Comment:
Important: `read -r -a` stops at the first newline, so only the first line
of `USER_OPTION` (`-j`) or of the `JAVA_OPTIONS` environment variable reaches
Java. The old unquoted expansion split on newlines too, so a multi-line value
(a YAML `|` block in Helm `javaOpts` or Compose, which the Docker entrypoints
pass as `-j "${JAVA_OPTS:-}"`) used to pass every option and now silently drops
all but the first line. The same pattern is at line 158 here,
start-hugegraph-pd.sh lines 157 and 163, and start-hugegraph-store.sh lines 216
and 222 (line 217 here also scans only the first line of `_JAVA_OPTIONS`).
The existing `test-launcher-gc.sh` in server-tests.yml covers this: its
override string spans two lines, and at this head it fails 6 cases
(`env-override` and `cli-override` for Server, PD and Store) because
`-XX:G1RSetUpdatingPauseTimePercent=2` is lost. It passes at the merge base.
Please read up to NUL instead of newline at each site:
```suggestion
read -r -d '' -a USER_OPTIONS <<< "${USER_OPTION}"
```
With that change in the three launchers, `test-launcher-gc.sh` passes.
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/stop-hugegraph-store.sh:
##########
@@ -27,20 +27,33 @@ abs_path() {
echo "$( cd -P "$( dirname "$SOURCE" )" && pwd )"
}
+while getopts "i:" arg; do
+ case ${arg} in
+ i) PID_FILE_OVERRIDE="$OPTARG" ;;
+ ?) echo "USAGE: $0 [-i pid_file]" && exit 1 ;;
+ esac
+done
+
BIN=$(abs_path)
TOP="$(cd $BIN/../ && pwd)"
. "$BIN"/util.sh
-PID_FILE=$BIN/pid
+reject_unsafe_path "$PID_FILE_OVERRIDE" "-i pid file" crlf || exit 1
Review Comment:
Important: The existing Store stop fixture now fails.
`test-stop-hugegraph-store.sh` (the "Verify Store stop failure propagation and
PID retention" step of the `struct` job in pd-store-ci.yml) replaces util.sh
with a stub that defines only `kill_process_and_wait`, so this new
`reject_unsafe_path` call exits with `command not found` and the
`STOP_RESULT=0` case returns 1 instead of removing the pid file. It passes at
the merge base and fails at this head on Linux (bash 5.2). This is separate
from the signal and preload fixtures raised earlier. Please add
`reject_unsafe_path` and `canonicalize_file` to that fixture's stub (or source
the real util.sh and stub only `kill_process_and_wait`), keeping its existing
assertions.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:
##########
@@ -71,12 +101,21 @@ if [[ "$OPEN_SECURITY_CHECK" != "true" &&
"$OPEN_SECURITY_CHECK" != "false" ]];
exit_with_usage_help
fi
+# Verify the config files exist before reading them
+ensure_file_readable "$CONF/rest-server.properties" "REST server config" ||
exit 1
+ensure_file_readable "$CONF/gremlin-server.yaml" "Gremlin server config" ||
exit 1
Review Comment:
Important: This new check breaks the existing Docker entrypoint process
fixture. `test-docker-entrypoint-process.sh`, which `docker-entrypoint-test.sh`
runs in the server-tests.yml "Validate Docker integration" step, builds a conf
dir with only `rest-server.properties` and `graphs/hugegraph.properties`, so
start-hugegraph.sh now exits 1 with `ERROR: missing Gremlin server config`
before it launches the stub server, and the fixture stops at `entrypoint exited
before .../exit0/child-running`. On Linux it passes at the merge base and fails
at this head; adding a `conf/gremlin-server.yaml` to the fixture makes it pass
again. Please add that file to the fixture (the check itself is fine for real
installs, which always ship 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
