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.

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

[CLOUD-3188] Move GC logs to PREPEND_JAVA_OPTS to bypass the specific EAP checks #141

Merged
merged 1 commit into from Dec 10, 2019

Conversation

yersan
Copy link
Contributor

@yersan yersan commented Dec 5, 2019

The solution is a bit hacky, but I haven't found a better approach, so maybe this one is acceptable.

EAP in standalone.sh will avoid adding any GC log file configuration if there is any JAVA_OPTS setting related to GC logs. This check clashes with the jvm_specific_diagnostics for JDK 11, where the -Xlog:gc::utctime decorator is enabled.

To prevent EAP skips the GC log file configuration due to the jvm_specific_diagnostics settings, we could remove the settings from the JAVA_OPTS and add them to PREPEND_JAVA_OPTS. That change will bypass the EAP specific checks.

jira issue: https://issues.redhat.com/browse/CLOUD-3188

@jfdenise
Copy link
Contributor

jfdenise commented Dec 9, 2019

+1, looks fine.
With your fix in, we have both gc logs in console and in gc.log file. That is allowed (we can have multiple -Xlog property set in command line).
I understand the motivation of the check in standalone.sh but this check doesn't seem right: https://github.com/wildfly/wildfly-core/blob/master/core-feature-pack/src/main/resources/content/bin/standalone.sh#L271
It does check for OpenJDK8 and OpenJDK 9+ Xlog:gc although we could have for example -Xlog:age,gc. Furthermore the added VM properties are also for OpenJ9.
https://github.com/wildfly/wildfly-core/blob/master/core-feature-pack/src/main/resources/content/bin/standalone.sh#L283

This seems a bug to me.

# CLOUD-3188 if JAVA_DIAGNOSTICS and the JDK specific diagnostics contain any GC log configuration, move the settings to PREPEND_JAVA_OPTS
# to bypass the specific EAP checks done on JAVA_OPTS in standalone.sh that could remove the GC EAP specific log configurations
JDK_DIAGNOSTICS=$(jvm_specific_diagnostics)
if [ "x$JAVA_DIAGNOSTICS" != "x" ] && [ "x{JDK_DIAGNOSTICS}" != "x" ] && grep -q "\-Xlog:gc" <<< "${JDK_DIAGNOSTICS}"; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the grep part could fail in case JDK_DIAGNOSTICS change the syntax. Shouldn't we move the JDK_DIAGNOSTICS in all cases to PREPEND_JAVA_OPTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did in that way to apply the changes only when it is strictly necessary based on the current standalone.sh, and hence to minimize any possible side effects when the JDK_DIAGNOSTICS does not contain a -Xlog:gc setting.

However, I agree @jfdenise. Indeed, we should end up without duplicates in the JAVA_OPTS so any bits adding the same settings after processing this, does not make sense, so order here is irrelevant. Well, the -Xlog settings are a bit more tricky in this aspect since the same setting is overwritten by the following -Xlog argument configuring the same aspect. e.g if JDK_DIAGNOSTICS defines a log file by using -Xlog:gc:file=/one/path and the standalone.sh adds a different path using -Xlog:gc:file=/other/path, the latest configuration wins. It could make sense in this context, although enabling JAVA_DIAGNOSTICS is a user choice, so I could expect to see how those settings overrides the final JAVA_OPTS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just removed the grep check and appending the jvm specific diagnostics if it was activated and there is one

@jfdenise jfdenise merged commit ebde0d3 into wildfly:master Dec 10, 2019
@yersan yersan deleted the CLOUD-3188 branch December 10, 2019 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants