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

SOLR-17531: Use new unified GC options #2815

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

malliaridis
Copy link
Contributor

@malliaridis malliaridis commented Oct 29, 2024

https://issues.apache.org/jira/browse/SOLR-17531

Description

This is a follow-up to #2792 for fixing issues and cleaning up GC_LOG_OPTS configurations.

Solution

Use the new unified logging and remove configuration for setting GC to G1 (since JDK 9 this is the default).

Tests

No tests were added due to it's complexity. If you have any recommendations for how to test GC configurations, please let me know.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Starting from JDK 9, G1 is the default GC.

This commit fixes the issue of unrecognized VM options.
@github-actions github-actions bot added documentation Improvements or additions to documentation start-scripts prometheus-exporter labels Oct 29, 2024
@malliaridis malliaridis changed the title Use new unified GC options SOLR-17531: Use new unified GC options Oct 29, 2024
Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. I love that it feels like a lot of "weight" has been taken out of these scripts. Having said that, I wonder if you should drop a note to [email protected] asking for another set of eyes? Since GC is a dark art! With no tests!

@gerlowskija
Copy link
Contributor

Is this PR mostly about removing redundancy, or is it changing the effective defaults to be more in line with best-practices?

e.g. G1GC - are we moving away from it towards something else, or is it merely disappearing from our scripts because the JVM itself uses it as a default, etc.

@malliaridis
Copy link
Contributor Author

I tried to keep the settings the same and remove redundancy / update to new notation. G1GC is the default GC since Java 9. With Java 21 we also have Generational ZGC which could be a nice feature to enable, but I am not familiar enough to tune the configuration. This is also why I was looking for GC experts to improve the existing configuration and make sure the config I "migrated" is free of bugs.

In JDK 17 support for some JDK 8 log-related configs has been dropped. See https://confluence.atlassian.com/confkb/unrecognized-jvm-gc-options-when-using-jdk-11-17-1002472841.html for details.

Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

I think I prefer the GC being explicit instead of relying on whatever JDK value is the default. The logging cleanup configs look good, but I would keep the G1GC configs.

@malliaridis
Copy link
Contributor Author

I think I prefer the GC being explicit instead of relying on whatever JDK value is the default. The logging cleanup configs look good, but I would keep the G1GC configs.

I was thinking the same when I was editing it, thinking that if the defaults may change under certain circumstances, then we will not have a "reproducible" state. But for the same reason we would have to include all other defaults as well.

I tried to understand why this was added in the first place, and it seemed to be some pre JDK 9 stuff, where G1GC was not the default GC and had to be configured explicitly. Since we had a lot of legacy JDK configuration, I decided to remove it during the cleanup. Not sure if there were other reasons though, as I never had to configure the JDK before (besides memory).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation prometheus-exporter start-scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants