imbajin commented on code in PR #3253:
URL: https://github.com/apache/hugegraph/pull/3253#discussion_r4216789227
##########
hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh:
##########
@@ -174,11 +186,11 @@ case "$GC_OPTION" in
-XX:+UnlockDiagnosticVMOptions
-XX:-ZProactive"
;;
*)
- echo "Unrecognized gc option: '$GC_OPTION', default use g1, options
only support 'ZGC' now" >> ${OUTPUT}
+ echo "Unrecognized gc option: '$GC_OPTION', default use g1, options
only support 'ZGC' now" >> "${OUTPUT}"
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
-Dfastjson.parser.safeMode=true -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:
[P2] Spaces in overridden config/log paths are still split when launching
the JVM
CONF/LOGS are interpolated into the JVM_OPTIONS string, but the later exec
expands ${JVM_OPTIONS} / ${JAVA_OPTIONS} without quoting. Even with a normal
installation path, passing -c '/data/external conf' or -l '/data/external logs'
creates extra standalone arguments. Using the original PD, Store and Server
launchers at e272e27c96f733f88f6b2e1163be0b10a7b8a7d4, I captured standalone
conf/log4j2.xml / logs arguments. Replaying the captured argv with JDK 21 made
all three entry points exit with “Could not find or load main class
conf.log4j2.xml”. Quoting the paths and redirections elsewhere therefore does
not yet make the space-containing directories supported by this PR usable at
startup.
Please keep script-generated JVM arguments in an array and pass each
argument separately to Java, while preserving the existing parsing contract for
user-supplied Java options. The final Server config-file arguments and
classpath also need to remain single arguments. Please add a regression
covering the actual launcher argv, rather than only the canonicalize/cron
helpers.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/stop-monitor.sh:
##########
@@ -25,11 +25,15 @@ function abs_path() {
echo "$( cd -P "$( dirname "$SOURCE" )" && pwd )"
}
-BIN=`abs_path`
-TOP="$(cd $BIN/../ && pwd)"
+BIN="$(abs_path)"
+TOP="$(cd "$BIN"/../ && pwd)"
-. $BIN/util.sh
+. "$BIN"/util.sh
-CRONTAB_JOB="$TOP/bin/monitor-hugegraph.sh"
+# Must equal the serialized form start-monitor.sh saved (one quoted shell
word),
+# otherwise grep -F cannot match a path containing an apostrophe or space and
the
+# job could never be removed. shell_quote (not cron_quote) so removal is not
+# blocked by validation.
+CRONTAB_JOB="$(shell_quote "$TOP/bin/monitor-hugegraph.sh")"
Review Comment:
[P2] Monitor removal must also handle cron entries created before the upgrade
The new removal key always includes surrounding single quotes, while the old
start-monitor.sh wrote an unquoted $TOP/bin/monitor-hugegraph.sh. After an
in-place upgrade that preserves crontab, those strings do not match, but
crontab_remove still returns success. I reproduced this using the base
version's actual start-monitor.sh and the current stop-monitor.sh with a
temporary crontab stub: stop returned 0 and left the old monitor entry intact.
Running the new start-monitor.sh first added a second entry for the same path;
stop then removed only the new entry.
As a result, stop-hugegraph.sh can report that monitoring has stopped while
the old cron job may restart the service on the next minute. Please safely
recognize or migrate the legacy format when adding/removing entries, and add an
upgrade regression covering “old-version creation → new-version removal”, while
retaining the existing tests for spaces and single quotes in paths.
The reproduction above is bound to e272e27c96f733f88f6b2e1163be0b10a7b8a7d4.
--
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]
