byteayan commented on code in PR #3258:
URL: https://github.com/apache/hugegraph/pull/3258#discussion_r4173761270
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -121,13 +121,43 @@ if [ "$JAVA_OPTIONS" = "" ]; then
echo "Failed to start HugeGraphServer, requires at least ${MIN_MEM}MB
free memory" >> "${OUTPUT}"
exit 1
fi
- JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m -XX:+HeapDumpOnOutOfMemoryError
-XX:HeapDumpPath=${LOGS} ${USER_OPTION}"
+ JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m ${USER_OPTION}"
# Rolling out detailed GC logs
#JAVA_OPTIONS="${JAVA_OPTIONS} -XX:+UseGCLogFileRotation
-XX:GCLogFileSize=10M -XX:NumberOfGCLogFiles=3 \
# -Xloggc:./logs/gc.log -XX:+PrintHeapAtGC
-XX:+PrintGCDetails -XX:+PrintGCDateStamps"
fi
+# Keep heap dumps and JVM crash logs in $LOGS whatever JAVA_OPTIONS holds.
They go
+# first so a value from JAVA_OPTIONS, -j or _JAVA_OPTIONS, all later on the
command
+# line, still wins. JAVA_TOOL_OPTIONS and JDK_JAVA_OPTIONS are read before the
command
+# line, so a flag set there is left out here instead of being overridden.
+# A restarted container often reuses the JVM's PID, and HotSpot will not
overwrite an
+# existing crash log or heap dump, so the file names also carry the launch
time. The
+# script execs java below, so $$ is the JVM's PID.
+LAUNCH_STAMP=$(date +%Y%m%d-%H%M%S)
+CRASH_OPTIONS=""
+SET_HEAP_DUMP="true"
+SET_HEAP_DUMP_PATH="true"
+SET_ERROR_FILE="true"
+for OPTION in ${JAVA_TOOL_OPTIONS:-} ${JDK_JAVA_OPTIONS:-}; do
Review Comment:
Done in ac9d98b0. The defaults now go first in `JAVA_TOOL_OPTIONS`, and the
shell scan is gone, so the JVM handles precedence and parsing for the
operator's `JAVA_TOOL_OPTIONS`, `JDK_JAVA_OPTIONS` (including `@argfile`), the
command line and `_JAVA_OPTIONS`. The test now checks the values a real JVM
resolves for a quoted opt-out, an `@argfile`, flag text inside a property value
and operator paths in `JAVA_OPTIONS`.
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-java-security-properties.sh:
##########
@@ -432,6 +442,85 @@ assert_argument "true" "$ENABLED_CAPTURE"
assert_argument "-Doperator.marker=preserved" "$ENABLED_CAPTURE"
assert_no_argument '^-D(networkaddress\.cache\.ttl|sun\.net\.inetaddr\.ttl)=' \
"$ENABLED_CAPTURE"
+assert_argument "-XX:+HeapDumpOnOutOfMemoryError" "$ENABLED_CAPTURE"
+# The names carry the launch time, since a restarted container often reuses the
+# PID and HotSpot will not overwrite an existing crash log or heap dump.
+LOGS_PATTERN=$(printf '%s' "${SERVER_ROOT}/logs" | sed
's/[][\.*^$+?(){}|]/\\&/g')
+assert_argument_matching \
+
"^-XX:HeapDumpPath=${LOGS_PATTERN}/java_pid[0-9]+_[0-9]{8}-[0-9]{6}\.hprof$" \
+ "$ENABLED_CAPTURE"
+assert_argument_matching \
+ "^-XX:ErrorFile=${LOGS_PATTERN}/hs_err_pid%p_[0-9]{8}-[0-9]{6}\.log$" \
+ "$ENABLED_CAPTURE"
+
+# Heap dumps and JVM crash logs must stay in logs/ when JAVA_OPTIONS replaces
+# the default heap options, and an operator's own paths must still win.
+CUSTOM_OPTIONS_CAPTURE="${TEMP_DIR}/custom-java-options.args"
+CAPTURE_FILE="$CUSTOM_OPTIONS_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ JAVA_OPTIONS="-Xmx1g -XX:ErrorFile=/operator/hs_err.log \
+ -XX:HeapDumpPath=/operator/dumps" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true
>/dev/null
+
+assert_argument "-Xmx1g" "$CUSTOM_OPTIONS_CAPTURE"
+assert_argument "-XX:+HeapDumpOnOutOfMemoryError" "$CUSTOM_OPTIONS_CAPTURE"
+assert_argument_matching "^-XX:HeapDumpPath=${LOGS_PATTERN}/"
"$CUSTOM_OPTIONS_CAPTURE"
+LAST_ERROR_FILE_ARGUMENT=$(grep -E '^-XX:ErrorFile=' \
+ "$CUSTOM_OPTIONS_CAPTURE" | tail -n 1)
+if [[ "$LAST_ERROR_FILE_ARGUMENT" != "-XX:ErrorFile=/operator/hs_err.log" ]];
then
+ fail "operator -XX:ErrorFile was overridden by the launcher default"
+fi
+LAST_HEAP_DUMP_PATH_ARGUMENT=$(grep -E '^-XX:HeapDumpPath=' \
+ "$CUSTOM_OPTIONS_CAPTURE" | tail -n 1)
+if [[ "$LAST_HEAP_DUMP_PATH_ARGUMENT" != "-XX:HeapDumpPath=/operator/dumps"
]]; then
+ fail "operator -XX:HeapDumpPath was overridden by the launcher default"
+fi
+
+# JAVA_TOOL_OPTIONS and JDK_JAVA_OPTIONS are read before the command line, so
+# the launcher must leave out a flag already set there rather than override it.
+TOOL_OPTIONS_CAPTURE="${TEMP_DIR}/tool-java-options.args"
+CAPTURE_FILE="$TOOL_OPTIONS_CAPTURE" JAVA_HOME="$MOCK_JAVA_HOME" \
+ JAVA_TOOL_OPTIONS="-XX:ErrorFile=/operator/hs_err.log" \
+ JDK_JAVA_OPTIONS="-XX:-HeapDumpOnOutOfMemoryError" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true
>/dev/null
+
+assert_no_argument '^-XX:ErrorFile=' "$TOOL_OPTIONS_CAPTURE"
+assert_no_argument '^-XX:[+-]HeapDumpOnOutOfMemoryError$'
"$TOOL_OPTIONS_CAPTURE"
+assert_argument_matching "^-XX:HeapDumpPath=${LOGS_PATTERN}/"
"$TOOL_OPTIONS_CAPTURE"
+
+# With telemetry on, the launcher adds its agent to JAVA_TOOL_OPTIONS. It must
+# keep the operator's flags there, or the -XX:ErrorFile left out above is lost.
+OT_EXPECTED_MD5=$(grep -E '^ *expected_md5=' "$SERVER_SCRIPT" | cut -d'"' -f2)
+MOCK_MD5_BIN="${TEMP_DIR}/mock-md5-bin"
+mkdir -p "$MOCK_MD5_BIN"
+printf '#!/bin/bash\necho "%s $1"\n' "$OT_EXPECTED_MD5" >
"${MOCK_MD5_BIN}/md5sum"
+chmod +x "${MOCK_MD5_BIN}/md5sum"
+OT_JAR_PATH="${SERVER_ROOT}/plugins/opentelemetry-javaagent.jar"
+OT_JAR_EXISTED="false"
+if [[ -e "$OT_JAR_PATH" ]]; then
+ OT_JAR_EXISTED="true"
+else
+ mkdir -p "${SERVER_ROOT}/plugins"
+ : > "$OT_JAR_PATH"
+fi
+TELEMETRY_CAPTURE="${TEMP_DIR}/telemetry.args"
+TELEMETRY_TOOL_OPTIONS="${TEMP_DIR}/telemetry-tool-options.txt"
+CAPTURE_FILE="$TELEMETRY_CAPTURE"
TOOL_OPTIONS_CAPTURE_FILE="$TELEMETRY_TOOL_OPTIONS" \
+ JAVA_HOME="$MOCK_JAVA_HOME" PATH="${MOCK_MD5_BIN}:${PATH}" \
+ JAVA_TOOL_OPTIONS="-XX:ErrorFile=/operator/hs_err.log" \
+ STDOUT_MODE=true "$SERVER_SCRIPT" \
+ "${CONF}/gremlin-server.yaml" "${CONF}/rest-server.properties" true "" ""
true \
+ >/dev/null
+if [[ "$OT_JAR_EXISTED" == "false" ]]; then
Review Comment:
Done in ac9d98b0: the EXIT cleanup removes the telemetry jar (only when the
test created it) and the log fixtures, so a failing launcher run no longer
leaves them in the distribution. I checked it by making the telemetry step exit
nonzero.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -121,13 +121,43 @@ if [ "$JAVA_OPTIONS" = "" ]; then
echo "Failed to start HugeGraphServer, requires at least ${MIN_MEM}MB
free memory" >> "${OUTPUT}"
exit 1
fi
- JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m -XX:+HeapDumpOnOutOfMemoryError
-XX:HeapDumpPath=${LOGS} ${USER_OPTION}"
+ JAVA_OPTIONS="-Xms${MIN_MEM}m -Xmx${XMX}m ${USER_OPTION}"
# Rolling out detailed GC logs
#JAVA_OPTIONS="${JAVA_OPTIONS} -XX:+UseGCLogFileRotation
-XX:GCLogFileSize=10M -XX:NumberOfGCLogFiles=3 \
# -Xloggc:./logs/gc.log -XX:+PrintHeapAtGC
-XX:+PrintGCDetails -XX:+PrintGCDateStamps"
fi
+# Keep heap dumps and JVM crash logs in $LOGS whatever JAVA_OPTIONS holds.
They go
+# first so a value from JAVA_OPTIONS, -j or _JAVA_OPTIONS, all later on the
command
+# line, still wins. JAVA_TOOL_OPTIONS and JDK_JAVA_OPTIONS are read before the
command
+# line, so a flag set there is left out here instead of being overridden.
+# A restarted container often reuses the JVM's PID, and HotSpot will not
overwrite an
+# existing crash log or heap dump, so the file names also carry the launch
time. The
+# script execs java below, so $$ is the JVM's PID.
+LAUNCH_STAMP=$(date +%Y%m%d-%H%M%S)
+CRASH_OPTIONS=""
+SET_HEAP_DUMP="true"
+SET_HEAP_DUMP_PATH="true"
+SET_ERROR_FILE="true"
+for OPTION in ${JAVA_TOOL_OPTIONS:-} ${JDK_JAVA_OPTIONS:-}; do
+ case "${OPTION}" in
+ -XX:[+-]HeapDumpOnOutOfMemoryError) SET_HEAP_DUMP="false" ;;
+ -XX:HeapDumpPath=*) SET_HEAP_DUMP_PATH="false" ;;
+ -XX:ErrorFile=*) SET_ERROR_FILE="false" ;;
+ esac
+done
+if [[ $SET_HEAP_DUMP == "true" ]]; then
+ CRASH_OPTIONS="${CRASH_OPTIONS} -XX:+HeapDumpOnOutOfMemoryError"
+fi
+if [[ $SET_HEAP_DUMP_PATH == "true" ]]; then
+ CRASH_OPTIONS="${CRASH_OPTIONS}
-XX:HeapDumpPath=${LOGS}/java_pid$$_${LAUNCH_STAMP}.hprof"
Review Comment:
Done in ac9d98b0: if a heap dump or crash log with that launch time already
exists in `$LOGS`, the name gets a counter (`_<stamp>-1`, `-2`, ...), so a
restart in the same second with the same PID gets a new file. The test covers
it with a fixed clock and existing files.
--
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]
