bitflicker64 commented on code in PR #3305:
URL: https://github.com/apache/hugegraph/pull/3305#discussion_r4236981956


##########
hugegraph-pd/hg-pd-dist/src/assembly/static/bin/start-hugegraph-pd.sh:
##########
@@ -106,10 +106,15 @@ if [ "$JAVA_OPTIONS" = "" ]; then
     #              -Xloggc:./logs/gc.log -XX:+PrintHeapAtGC 
-XX:+PrintGCDetails -XX:+PrintGCDateStamps"
 fi
 
-# Using G1GC as the default garbage collector (Recommended for large memory 
machines)
+# Keep JVM/caller GC selection by default; explicitly select G1 when requested.
 case "$GC_OPTION" in
-    "")
-        echo "Using G1GC as the default garbage collector"
+    ""|g1|G1)
+        if [[ "$GC_OPTION" == g1 || "$GC_OPTION" == G1 ]]; then
+            echo "Using G1GC"
+            JAVA_OPTIONS="${JAVA_OPTIONS} -XX:+UseG1GC"
+        else
+            echo "Using JVM garbage collector configuration"
+        fi
         JAVA_OPTIONS="${JAVA_OPTIONS} -XX:+ParallelRefProcEnabled \

Review Comment:
   Minor: the launcher tuning is appended after the caller's options, so it 
overrides them. `-j` lands in `JAVA_OPTIONS` at line 102 and this line then 
appends `-XX:InitiatingHeapOccupancyPercent=50`. The G1 example in 
`hugegraph-pd/docs/configuration.md` lines 511-514 (`-g g1 -j "... 
-XX:InitiatingHeapOccupancyPercent=45"`), which starts for the first time with 
this PR, therefore runs with 50, not 45. Checked on OpenJDK 17.0.20: `java 
-XX:+UseG1GC -XX:InitiatingHeapOccupancyPercent=45 
-XX:InitiatingHeapOccupancyPercent=50 -XX:+PrintFlagsFinal -version` reports 
`InitiatingHeapOccupancyPercent = 50 {command line}`. The ordering predates 
this PR (the no `-g` path and the Docker entrypoint's `-j "${JAVA_OPTS}"` 
behave the same), and the Store launcher has the same shape at line 182. 
Requested change: put the launcher defaults before the caller's options so `-j` 
and `JAVA_OPTIONS` win, or correct the documented example.



##########
hugegraph-pd/hg-pd-dist/src/assembly/static/bin/start-hugegraph-pd.sh:
##########
@@ -106,10 +106,15 @@ if [ "$JAVA_OPTIONS" = "" ]; then
     #              -Xloggc:./logs/gc.log -XX:+PrintHeapAtGC 
-XX:+PrintGCDetails -XX:+PrintGCDateStamps"
 fi
 
-# Using G1GC as the default garbage collector (Recommended for large memory 
machines)
+# Keep JVM/caller GC selection by default; explicitly select G1 when requested.
 case "$GC_OPTION" in
-    "")
-        echo "Using G1GC as the default garbage collector"
+    ""|g1|G1)

Review Comment:
   Minor: the Server launcher still rejects the same documented value. 
`hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh` 
lines 140-156 at this head keep the old `""` / `zgc|ZGC` / `*` case, so 
`bin/start-hugegraph.sh -g g1` (which forwards `GC_OPTION` as the fifth 
argument) still writes `Unrecognized gc option: 'g1'` and exits 1, while the 
usage text in `hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh` 
line 658 advertises `[-g g1]`. After this PR the three launchers disagree on 
the same flag. Requested change: apply the same `""|g1|G1` branch to 
`hugegraph-server.sh` here, or say in the description that Server is left for a 
follow-up.



-- 
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]

Reply via email to