-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated java options manage. Fixes #229 #231
base: main
Are you sure you want to change the base?
Conversation
Difference between old and new opts list NEW Full lists OLD NEW |
extra java options are added at the end of the jvm args so user can override defaults extraJvmOpts: "-XX:MaxRAMPercentage=75.0" OLD NEW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bunch of JAVA_
env variables in the image descriptor files that are no longer valid. I think we need to remove them as part of this PR.
if [ -z "${CONTAINER_MAX_MEMORY:-}" ]; then | ||
return | ||
fi | ||
source "$JBOSS_CONTAINER_UTIL_LOGGING_MODULE/logging.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this script provide us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it provides logging facilities in case of debugging
# This has forked (and diverged) from: | ||
# at https://github.com/fabric8io/run-java-sh | ||
# | ||
# ========================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still relevant or are we using https://github.com/jboss-container-images/openjdk/blob/ubi9/modules/run/artifacts/opt/jboss/container/java/run/run-java.sh ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ported from the original script, I guess it can be removed from here
@ryanemerson do you think are we ok with the new GC settings? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rigazilla There are some minor spacing issues in the PR and outdated JAVA vars in server-openjdk.yaml including JAVA_MAX_MEMORY_RATIO
defaulting to 50%. That default has been changed for UBI9 OpenJDKs to 80% and I wonder if should let the default default to overtake for consistency.
I also wonder what documentation changes are required with this PR. If any.
if [ -n "${DEBUG:-}" ]; then | ||
set -x | ||
set -x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect spacing
init_limit_env_vars | ||
# Echo options, trimming trailing and multiple spaces | ||
echo "${JAVA_OPTIONS:-} $(calc_init_memory) $(calc_max_memory) $(diagnostics_options) $(gc_options)" | awk '$1=$1' | ||
get_java_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect spacing
Updates java-opts.sh with the latest from run-java.sh
unfortunately run-java.sh funcs cannot be imported without execute the script, so copy is needed