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

Introduce system JVM options #48252

Merged
merged 9 commits into from
Oct 28, 2019
Merged

Conversation

jasontedor
Copy link
Member

This commit moves JVM options that we are setting on behalf of the user that we do not expect them to fiddle with out of the jvm.options configuration file and into the JVM options parser. In this way, we discourage fiddling with these settings, but more importantly, we ensure that as we evolve or add to these settings that a user would pick these pick instead of being left behind if they have a modified jvm.options file and do not pick any new that come with the distribution.

Closes #48222

This commit moves JVM options that we are setting on behalf of the user
that we do not expect them to fiddle with out of the jvm.options
configuration file and into the JVM options parser. In this way, we
discourage fiddling with these settings, but more importantly, we ensure
that as we evolve or add to these settings that a user would pick these
pick instead of being left behind if they have a modified jvm.options
file and do not pick any new that come with the distribution.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

LGTM. Excited to see the upgrade experience improvements that implicitly result from this!

@jasontedor
Copy link
Member Author

@elasticmachine update branch

@@ -90,8 +91,10 @@ public void accept(final int lineNumber, final String line) {
final List<String> substitutedJvmOptions =
substitutePlaceholders(jvmOptions, Map.of("ES_TMPDIR", System.getenv("ES_TMPDIR")));
final List<String> ergonomicJvmOptions = JvmErgonomics.choose(substitutedJvmOptions);
substitutedJvmOptions.addAll(ergonomicJvmOptions);
final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(substitutedJvmOptions);
final List<String> finalJvmOptions = Stream.concat(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered if this would be any better using a plain old list:

final List<String> finalJvmOptions = new ArrayList<>(SystemJvmOptions.systemJvmOptions());
finalJvmOptions.addAll(substitutedJvmOptions);
finalJvmOptions.addAll(ergonomicJvmOptions);

Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

The packaging tests do some fiddling with jvm.options, do they need tweaked along with these changes?

@jasontedor
Copy link
Member Author

@elasticmachine update branch

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

-Dlog4j.shutdownHookEnabled=false
-Dlog4j2.disable.jmx=true

## JVM temporary directory
-Djava.io.tmpdir=${ES_TMPDIR}
Copy link
Member

Choose a reason for hiding this comment

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

Since the environment variable ES_TMPDIR is how users set the tempdir, shouldn't this jvm option be included in the system options?


final class SystemJvmOptions {

static List<String> systemJvmOptions() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why a separate class and not just a static final list in JvmOptionsParser?

@jasontedor
Copy link
Member Author

@elasticmachine run elasticsearch-ci/2

* elastic/master:
  [Docs] Fix opType options in IndexRequest API example. (elastic#48290)
  Simplify Shard Snapshot Upload Code (elastic#48155)
  Mute ClassificationIT tests (elastic#48338)
  Reenable azure repository tests and remove some randomization in http servers  (elastic#48283)
  Use an env var for the classpath of jar hell task (elastic#48240)
  Refactor FIPS BootstrapChecks to simple checks (elastic#47499)
  Add "format" to "range" queries resulted from optimizing a logical AND (elastic#48073)
  [DOCS][Transform] document limitation regarding rolling upgrade with 7.2, 7.3 (elastic#48118)
  Fail with a better error when if there are no ingest nodes (elastic#48272)
  Fix executing enrich policies stats (elastic#48132)
  Use MultiFileTransfer in CCR remote recovery (elastic#44514)
  Make BytesReference an interface (elastic#48171)
  Also validate source index at put enrich policy time. (elastic#48254)
  Add 'javadoc' task to lifecycle check tasks (elastic#48214)
  Remove option to enable direct buffer pooling (elastic#47956)
  [DOCS] Add 'Selecting gateway and seed nodes' section to CCS docs (elastic#48297)
  Add Enrich Origin (elastic#48098)
  fix incorrect comparison (elastic#48208)
@jasontedor jasontedor merged commit 2c18cbf into elastic:master Oct 28, 2019
jasontedor added a commit that referenced this pull request Oct 28, 2019
This commit moves JVM options that we are setting on behalf of the user
that we do not expect them to fiddle with out of the jvm.options
configuration file and into the JVM options parser. In this way, we
discourage fiddling with these settings, but more importantly, we ensure
that as we evolve or add to these settings that a user would pick these
pick instead of being left behind if they have a modified jvm.options
file and do not pick any new that come with the distribution.
jasontedor added a commit that referenced this pull request Oct 28, 2019
This commit moves JVM options that we are setting on behalf of the user
that we do not expect them to fiddle with out of the jvm.options
configuration file and into the JVM options parser. In this way, we
discourage fiddling with these settings, but more importantly, we ensure
that as we evolve or add to these settings that a user would pick these
pick instead of being left behind if they have a modified jvm.options
file and do not pick any new that come with the distribution.
@jasontedor jasontedor deleted the system-jvm-options branch October 28, 2019 11:59
danielmitterdorfer added a commit to danielmitterdorfer/rally-teams that referenced this pull request Mar 13, 2020
With this commit we remove all JVM options that are considered "system
JVM options" which are already hardcoded in Elasticsearch and set by
default.

Relates elastic/elasticsearch#48252
danielmitterdorfer added a commit to elastic/rally-teams that referenced this pull request Mar 19, 2020
With this commit we remove all JVM options that are considered "system
JVM options" which are already hardcoded in Elasticsearch and set by
default.

Relates elastic/elasticsearch#48252
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jvm.options after upgrade experience
6 participants