bitflicker64 commented on code in PR #3253:
URL: https://github.com/apache/hugegraph/pull/3253#discussion_r4145868204
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -105,17 +100,26 @@ if [ -z "$DAEMON" ]; then
DAEMON="true"
fi
-while getopts "d:g:j:y:" arg; do
+while getopts "d:c:g:i:j:l:y:" arg; do
case ${arg} in
+ c) CONF_OVERRIDE="$OPTARG" ;;
g) GC_OPTION="$OPTARG" ;;
+ i) PID_FILE_OVERRIDE="$OPTARG" ;;
j) USER_OPTION="$OPTARG" ;;
+ l) LOGS_OVERRIDE="$OPTARG" ;;
# Telemetry is used to collect metrics, traces and logs
y) OPEN_TELEMETRY="$OPTARG" ;;
d) DAEMON="$OPTARG" ;;
- ?) echo "USAGE: $0 [-d true|false] [-g g1] [-j xxx] [-y true|false]"
&& exit 1 ;;
+ ?) echo "USAGE: $0 [-d true|false] [-c conf_dir] [-g g1] [-i pid_file]
[-j opts] [-l logs_dir] [-y true|false]" && exit 1 ;;
esac
done
+CONF="${CONF_OVERRIDE:-$TOP/conf}"
+LOGS="${LOGS_OVERRIDE:-$TOP/logs}"
+PLUGINS="$TOP/plugins"
Review Comment:
Important: `PLUGINS` still points at `$TOP/plugins`, and line 124 calls
`ensure_path_writable "$PLUGINS"`, which runs `mkdir -p` and then exits 1 with
"No write permission on directory" when the directory is not writable. On the
read-only install that #3236 and this PR target, Store exits here before Java
starts, even when `-c`, `-l` and `-i` all point at writable directories. PD has
the same call at start-hugegraph-pd.sh line 70 and Server at
hugegraph-server.sh line 61, so none of the three roles can start from a
read-only install. The PR title also says plugins paths are overridable, but no
plugins override exists. Please either add a `PLUGINS_OVERRIDE` (flag plus env,
exported to hugegraph-server.sh like the others) or only require `$PLUGINS` to
be writable when it has to be written, for example when `-y true` needs to
download the OpenTelemetry agent.
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -175,7 +179,7 @@ case "$GC_OPTION" in
exit 1
esac
-JVM_OPTIONS="-Dlog4j.configurationFile=${CONF}/log4j2.xml
-Dfastjson.parser.safeMode=true
-Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager"
+JVM_OPTIONS="-Dlog4j.configurationFile=${CONF}/log4j2.xml
-Dlogging.config=${CONF}/log4j2.xml -DLOG_PATH=${LOGS} -Dbolt.log.path=${LOGS}
-Dlogging.path=${LOGS}
-Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager"
Review Comment:
Important: This line drops `-Dfastjson.parser.safeMode=true`, which the old
`JVM_OPTIONS` passed to every Store JVM. The flag is not related to conf, logs
or pid paths. fastjson 1.2.83 is still on the Store classpath through
hg-store-core and hg-store-rocksdb, and `StoreNodeWrapper` in
hugegraph-clustertest-minicluster still sets the flag, so after this change
packaged Store nodes run with fastjson autoType handling enabled while the test
cluster does not. Existing deployments lose this hardening on upgrade without
opting in. Please keep the flag:
```suggestion
JVM_OPTIONS="-Dlog4j.configurationFile=${CONF}/log4j2.xml
-Dlogging.config=${CONF}/log4j2.xml -DLOG_PATH=${LOGS} -Dbolt.log.path=${LOGS}
-Dlogging.path=${LOGS} -Dfastjson.parser.safeMode=true
-Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager"
```
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -153,7 +157,7 @@ if [ "$JAVA_OPTIONS" = "" ]; then
# JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m
-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=${LOGS} ${USER_OPTION}"
# Rolling out detailed GC logs
- JAVA_OPTIONS="${JAVA_OPTIONS}
-Xlog:gc=info:file=./logs/gc.log:time,uptime,level,tags:filecount=3,filesize=100m"
+ JAVA_OPTIONS="${JAVA_OPTIONS}
-Xlog:gc=info:file=./logs/gc.log:time,uptime,level,tags:filecount=3,filesize=100m"
Review Comment:
Important: The GC log still goes to `./logs/gc.log`, and the script runs `cd
${TOP}` at line 132, so it resolves to `$TOP/logs/gc.log` whatever `-l` says.
The JVM treats an unopenable `-Xlog` file as fatal. With JDK 17, `java
-Xlog:gc=info:file=/nonexistent_dir_x/gc.log -version` prints `Invalid -Xlog
option` and `Could not create the Java Virtual Machine`. The Store tarball
ships no `logs` directory (server-assembly.xml packages only `bin` and `conf`),
and with `-l` the script only creates the override directory. So a fresh
install started with `-l /var/log/hugegraph-store` and the default
`JAVA_OPTIONS` has no `$TOP/logs`, and the JVM exits before Store starts,
whether or not the install is read-only. The `JAVA_OPTIONS` workaround
mentioned in #3236 also replaces the heap and heap dump settings. This line
also changed its indentation to three spaces. Please use the resolved logs
directory:
```suggestion
JAVA_OPTIONS="${JAVA_OPTIONS}
-Xlog:gc=info:file=${LOGS}/gc.log:time,uptime,level,tags:filecount=3,filesize=100m"
```
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/stop-hugegraph-store.sh:
##########
@@ -27,12 +27,19 @@ 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 ;;
Review Comment:
Important: `restart-hugegraph-store.sh` sources this script and then
`start-hugegraph-store.sh` in the same shell with the same arguments. Before
this change the stop script ignored the arguments and the start script parsed
them. Now this `getopts "i:"` loop sees the start flags first, hits `?` on
`-d`, `-j`, `-g`, `-y`, `-c` or `-l`, and runs `exit 1`, which ends the whole
restart before anything is stopped or started. I reproduced it with these two
getopts loops sourced from a restart script: `restart.sh -j -Xmx4g` prints
`illegal option -- j` and exits 1, while the old stop script let start receive
`USER_OPTION=-Xmx4g`. Because the shell and `OPTIND` are shared, even `-i`
alone is consumed here and start skips its loop. Please make the stop script
tolerate the start flags when sourced (for example a silent optstring such as
`":i:c:d:g:j:l:y:"` that ignores unknown options) or change
restart-hugegraph-store.sh to run the two scripts as subprocesses, passing `-i`
to stop and all
flags to start.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:
##########
@@ -39,29 +39,36 @@ function abs_path() {
BIN=$(abs_path)
TOP="$(cd "$BIN"/../ && pwd)"
-CONF="$TOP/conf"
-LOGS="$TOP/logs"
SCRIPTS="$TOP/scripts"
-PID_FILE="$BIN/pid"
. "$BIN"/util.sh
# Note: keep ':' in the end of the string to indicate the option needs a value
-while getopts "d:g:m:p:s:j:t:y:" arg; do
- case ${arg} in
- d) DAEMON="$OPTARG" ;;
- g) GC_OPTION="$OPTARG" ;;
- m) OPEN_MONITOR="$OPTARG" ;;
- p) PRELOAD="$OPTARG" ;;
- s) OPEN_SECURITY_CHECK="$OPTARG" ;;
- j) USER_OPTION="$OPTARG" ;;
- t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
- # Telemetry is used to collect metrics, traces and logs
- y) OPEN_TELEMETRY="$OPTARG" ;;
- # Note: update usage info when the params changed
- ?) exit_with_usage_help ;;
- esac
-done
+while getopts "c:d:g:i:j:l:m:p:s:t:y:" arg; do
+ case ${arg} in
+ c) CONF_OVERRIDE="$OPTARG" ;;
+ d) DAEMON="$OPTARG" ;;
+ g) GC_OPTION="$OPTARG" ;;
+ i) PID_FILE_OVERRIDE="$OPTARG" ;;
+ j) USER_OPTION="$OPTARG" ;;
+ l) LOGS_OVERRIDE="$OPTARG" ;;
+ m) OPEN_MONITOR="$OPTARG" ;;
+ p) PRELOAD="$OPTARG" ;;
+ s) OPEN_SECURITY_CHECK="$OPTARG" ;;
+ t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
+ # Telemetry is used to collect metrics, traces and logs
+ y) OPEN_TELEMETRY="$OPTARG" ;;
+ # Note: update usage info when the params changed
+ ?) exit_with_usage_help ;;
+ esac
+ done
+
+CONF="${CONF_OVERRIDE:-$TOP/conf}"
+LOGS="${LOGS_OVERRIDE:-$TOP/logs}"
+PID_FILE="${PID_FILE_OVERRIDE:-$BIN/pid}"
+
+. "$BIN"/util.sh
Review Comment:
Minor: util.sh is already sourced at line 44 and only defines functions, so
this second `. "$BIN"/util.sh` does nothing and can go. The getopts block above
(lines 47 to 64) is also indented by five spaces with a stray leading space
before `done`, which the rest of the file does not do.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/start-hugegraph.sh:
##########
@@ -39,29 +39,36 @@ function abs_path() {
BIN=$(abs_path)
TOP="$(cd "$BIN"/../ && pwd)"
-CONF="$TOP/conf"
-LOGS="$TOP/logs"
SCRIPTS="$TOP/scripts"
-PID_FILE="$BIN/pid"
. "$BIN"/util.sh
# Note: keep ':' in the end of the string to indicate the option needs a value
-while getopts "d:g:m:p:s:j:t:y:" arg; do
- case ${arg} in
- d) DAEMON="$OPTARG" ;;
- g) GC_OPTION="$OPTARG" ;;
- m) OPEN_MONITOR="$OPTARG" ;;
- p) PRELOAD="$OPTARG" ;;
- s) OPEN_SECURITY_CHECK="$OPTARG" ;;
- j) USER_OPTION="$OPTARG" ;;
- t) SERVER_STARTUP_TIMEOUT_S="$OPTARG" ;;
- # Telemetry is used to collect metrics, traces and logs
- y) OPEN_TELEMETRY="$OPTARG" ;;
- # Note: update usage info when the params changed
- ?) exit_with_usage_help ;;
- esac
-done
+while getopts "c:d:g:i:j:l:m:p:s:t:y:" arg; do
+ case ${arg} in
+ c) CONF_OVERRIDE="$OPTARG" ;;
+ d) DAEMON="$OPTARG" ;;
+ g) GC_OPTION="$OPTARG" ;;
+ i) PID_FILE_OVERRIDE="$OPTARG" ;;
+ j) USER_OPTION="$OPTARG" ;;
+ l) LOGS_OVERRIDE="$OPTARG" ;;
+ m) OPEN_MONITOR="$OPTARG" ;;
Review Comment:
Important: `-m true` together with `-c`, `-l` or `-i` restarts the server
with the wrong paths. start-monitor.sh installs a cron job for
monitor-hugegraph.sh, which reads `$TOP/conf/rest-server.properties` for the
health URL, writes `$TOP/logs/monitor.log`, and on failure runs
`stop-hugegraph.sh false` and `start-hugegraph.sh -m false` with none of the
overrides. The restarted server then loads `$TOP/conf`, logs to `$TOP/logs` and
writes `$BIN/pid`, and the operator's `stop-hugegraph.sh -i <pid_file>` no
longer finds it. Please either reject `-m true` when an override is set, or
persist the overrides (for example in the cron line's environment) and use them
in monitor-hugegraph.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: [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]
