Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[OPENJDK-78] Re-work JAVA_OPTS to override script-calculated java options #302

Merged
merged 3 commits into from
Jun 6, 2022
Merged

[OPENJDK-78] Re-work JAVA_OPTS to override script-calculated java options #302

merged 3 commits into from
Jun 6, 2022

Conversation

jmtd
Copy link
Member

@jmtd jmtd commented Jun 1, 2022

https://issues.redhat.com/browse/OPENJDK-78

This is a first attempt at a way of short-circuiting all Java options
generation.

@jmtd
Copy link
Member Author

jmtd commented Jun 1, 2022

I was thinking we needed to either continue to support JAVA_OPTS or JAVA_OPTS_APPEND in addition to the PR in its current state. Then, I wondered, what purpose does JAVA_OPTS serve at the moment anyway? Perhaps we should just let that override all auto-calculated options, and save introducing another variable.

@jmtd jmtd force-pushed the tuning-override branch 2 times, most recently from 245b31d to 1f9415e Compare June 1, 2022 13:30
@jmtd jmtd changed the title [OPENJDK-78] Add SKIP_OPTIONS to override all Java options [OPENJDK-78] Re-work JAVA_OPTS to override script-calculated java options Jun 1, 2022
@jmtd
Copy link
Member Author

jmtd commented Jun 1, 2022

The PR now better reflects how I remember JAVA_OPTS behaving in the dim and distant past.

@jmtd jmtd requested a review from jerboaa June 1, 2022 15:18
jerboaa
jerboaa reviewed Jun 1, 2022
View reviewed changes
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test which verifies that if JAVA_OPS and JAVA_OPTS_APPEND are both specified, then only JAVA_OPTS are being added. I.e. JAVA_OPTS_APPEND is an override for the default options. JAVA_OPTS are a replacement.

@jmtd
Copy link
Member Author

jmtd commented Jun 1, 2022

Good idea: I've just pushed one.

jerboaa
jerboaa approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please clarify the description of JAVA_OPTS_APPEND.

modules/jvm/module.yaml
example: "-verbose:class"
- name: JAVA_OPTS_APPEND
description: User specified Java options to be appended to generated options in JAVA_OPTS.
description: User specified Java options to be appended to the generated options.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add that this variable has no effect if JAVA_OPTS is set in the description.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind doing so, and I will. I originally decided not to because I wasn't also going to annotate all the other variables that are disabled by specifying JAVA_OPTS, but, due to their similar name and behaviour I can see that this one should be treated differently.

jmtd added 3 commits June 6, 2022 09:04
@jmtd
remove support for /opt/run-java-options
94b5829 
This is a very old and (I think) almost unused option to provide
additional JVM options via a file inside the container.

Signed-off-by: Jonathan Dowland <jdowland@redhat.com>
@jmtd
[OPENJDK-78] allow JAVA_OPTS to override options processing
ed65182 
If JAVA_OPTS is defined, don't attempt to expand the various
options we might otherwise provide: debug options, proxy settings,
JVM/GC tuning, etc.

https://issues.redhat.com/browse/OPENJDK-78

Signed-off-by: Jonathan Dowland <jdowland@redhat.com>
@jmtd
Add a test for JAVA_OPTS overriding JAVA_OPTS_APPEND
9e0e1c9 
Signed-off-by: Jonathan Dowland <jdowland@redhat.com>
@jmtd
Copy link
Member Author

jmtd commented Jun 6, 2022

Again, the failing test here passes locally and I think is sporadic on GH actions.

@jmtd jmtd merged commit 58f13e7 into jboss-container-images:ubi9 Jun 6, 2022
@jmtd jmtd deleted the tuning-override branch June 30, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerboaa jerboaa jerboaa approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jmtd @jerboaa