HDDS-14890. Improve ozone_add_default_gc_opts#10021
Conversation
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @AdyChechani for the patch.
| ozone_debug "No '-XX:...' jvm parameters are set. Adding safer GC settings '-XX:ParallelGCThreads=8 -XX:+UseConcMarkSweepGC -XX:NewRatio=3 -XX:CMSInitiatingOccupancyFraction=70 -XX:+CMSParallelRemarkEnabled' to the OZONE_OPTS" | ||
| else | ||
| ozone_debug "No '-XX:...' jvm parameters are set. Adding safer GC settings '-XX:ParallelGCThreads=8' to the OZONE_OPTS" |
There was a problem hiding this comment.
Please remove these ozone_debug messages. Since presence of -XX is not checked beforehand, these would always be printed. ozone_add_param already prints debug message:
ozone/hadoop-ozone/dist/src/shell/ozone/ozone-functions.sh
Lines 1137 to 1139 in 644a018
| { | ||
| local gc_opts | ||
| local java_major_version | ||
| java_major_version=$(ozone_get_java_major_version) |
There was a problem hiding this comment.
nit: please move this variable assignment into the if block to avoid running it unnecessarily for client commands
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @AdyChechani for updating the patch.
|
@AdyChechani Please try to avoid force-push when updating the PR. Here are some great articles that explain why: https://developers.mattermost.com/blog/submitting-great-prs/#4-avoid-force-pushing |
What changes were proposed in this pull request?
java_major_versiona local variable so it doesn’t leak into the broader shell environment.XXas thecheckstringtakes care of duplicate options, so there’s no need for the old[[ ! "$OZONE_OPTS" =~ "-XX" ]]check.ozone_errormessages, instead usedozone_debug.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14890
How was this patch tested?
Tested the function
ozone_add_default_gc_optsin isolation manually by creating a shell script locally, verifying that:OZONE_OPTS-XXoptions are added when one is already present