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

Don't drop user's MaxDirectMemorySize flag on jdk8/windows #48657

Conversation

williamrandolph
Copy link
Contributor

We had been testing whether a user had passed a value for MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal -version". If MaxDirectMemorySize equals zero, we set it to half of max heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly truncates values over 4g and returns multiples of 4g as zero. In order to always respect the user-defined settings, we need to check our input to see if an "-XX:MaxDirectMemorySize" value has been passed.

It may seem that the logic here could be slightly tighter: we could check for the MaxDirectMemorySize flag only on Windows/JDK8. But in practice, I believe that on all other systems, maxDirectMemorySize == 0 will only be true when a user sets MaxDirectMemorySize to 0 (or to a value greater than 8 exbibytes ((2^63)), which I believe not a presently realistic value).

Fixes #44174
Relates #48365

We had been testing whether a user had passed a value for
MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal
-version". If MaxDirectMemorySize equals zero, we set it to half of max
heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly
truncates values over 4g and returns multiples of 4g as zero. In order
to always respect the user-defined settings, we need to check our input
to see if an "-XX:MaxDirectMemorySize" value has been passed.
@elasticmachine
Copy link
Collaborator

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

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

As far as I understand the problem in #44174, this PR still won't solve it. If a heap size of e.g. 8GB is specified and the user sets -XX:MaxDirectMemorySize=4GB (or Elasticsearch ergonomically chooses 4GB), Elasticsearch would still behave as specified in #44174, wouldn't it? The only difference is that we will now correctly pass whatever the user has specified in -XX:MaxDirectMemorySize. However, as they have specified a value, they won't see the new warning message. Maybe we should show the warning message on Windows / JDK 8 always when maxDirectMemorySize is zero?

@williamrandolph
Copy link
Contributor Author

williamrandolph commented Oct 30, 2019

@danielmitterdorfer Thanks for the review.

On the current 7.x branch, with the following in config/jvm.options

-Xms8g
-Xmx8g
-XX:MaxDirectMemorySize=4g

We get a failure:

C:\Users\william_brafford\tasks\java8warning\es-7.6.0-SNAP-7.x> echo $env:JAVA_HOME
C:\Users\jenkins\.java\java8
C:\Users\william_brafford\tasks\java8warning\es-7.6.0-SNAP-7.x> .\bin\elasticsearch
future versions of Elasticsearch will require Java 11; your Java version from [C:\Users\jenkins\.java\java8\jre] does not meet this requirement
Warning: with JDK 8 on Windows, Elasticsearch may miscalculate MaxDirectMemorySize
  due to a JDK issue (JDK-8074459).
  Please use a newer version of Java or set MaxDirectMemorySize explicitly
Exception in thread "main" java.lang.OutOfMemoryError: Direct buffer memory
        at java.nio.Bits.reserveMemory(Bits.java:694)

But with the changes on this PR, we get a startup with no JDK issue warning.

JDK 8 is able to read a setting like -Xmx4g; the problem appears only when we parse output from java -XX:PrintFlagsFinal -version. So with this commit, we may read an incorrect value, but we won't end up using it or adding -XX:MaxDirectMemorySize=0 to the Java flags.

The unit test that I modified covers this behavior in a way, but it's not very clear. So I'll update that for clarity.

Finally, I agree with you that we should print some kind of warning even if the user is passing in a setting, because it would be good for users who are able to upgrade their JDK anyway. So I will adjust this a little bit. I don't want to instruct users to set the MaxDirectMemorySize when they've already done so; I think that would be confusing.

Even if a user has set MaxDirectMemorySize, they aren't future-proof for
this JDK bug. With this change, we issue a general warning for the
windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is
unset.
@williamrandolph
Copy link
Contributor Author

Now, when a Windows/JDK8 user fails to set MaxDirectMemorySize, they will see:

C:\Users\william_brafford\tasks\java8warning\es-7.6.0-SNAP-with-fix> .\bin\elasticsearch
future versions of Elasticsearch will require Java 11; your Java version from [C:\Users\jenkins\.java\java8\jre] does not meet this requirement
Warning: with JDK 8 on Windows, Elasticsearch may be unable to derive correct
  ergonomic settings due to a JDK issue (JDK-8074459). Please use a newer
  version of Java.
Warning: MaxDirectMemorySize may have been miscalculated due to JDK-8074459.
  Please use a newer version of Java or set MaxDirectMemorySize explicitly.

When they do set MaxDirectMemorySize, they'll see, e.g.:

C:\Users\william_brafford\tasks\java8warning\es-7.6.0-SNAP-with-fix> .\bin\elasticsearch
future versions of Elasticsearch will require Java 11; your Java version from [C:\Users\jenkins\.java\java8\jre] does not meet this requirement
Warning: with JDK 8 on Windows, Elasticsearch may be unable to derive correct
  ergonomic settings due to a JDK issue (JDK-8074459). Please use a newer
  version of Java.
[2019-10-30T21:51:48,406][INFO ][o.e.e.NodeEnvironment    ] [WBRAFFORD-WINDO] using [1] data paths, mounts [[(C:)]], net usable_space [239.4gb], net total_space [299.9gb], types [NTFS]
  [...]

…and so forth, on the way to a successful startup.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thanks for iterating! LGTM

@polyfractal polyfractal added v7.4.3 and removed v7.4.2 labels Oct 31, 2019
@williamrandolph williamrandolph merged commit 6da9735 into elastic:7.x Oct 31, 2019
@williamrandolph williamrandolph deleted the fix/7.x/pass-max-direct-memory-size branch October 31, 2019 19:10
williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Nov 11, 2019
…8657)

* Always pass user-specified MaxDirectMemorySize

We had been testing whether a user had passed a value for
MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal
-version". If MaxDirectMemorySize equals zero, we set it to half of max
heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly
truncates values over 4g and returns multiples of 4g as zero. In order
to always respect the user-defined settings, we need to check our input
to see if an "-XX:MaxDirectMemorySize" value has been passed.

* Always warn for Windows/jdk8 ergo issue

Even if a user has set MaxDirectMemorySize, they aren't future-proof for
this JDK bug. With this change, we issue a general warning for the
windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is
unset.
williamrandolph added a commit to williamrandolph/elasticsearch that referenced this pull request Nov 12, 2019
…8657)

* Always pass user-specified MaxDirectMemorySize

We had been testing whether a user had passed a value for
MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal
-version". If MaxDirectMemorySize equals zero, we set it to half of max
heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly
truncates values over 4g and returns multiples of 4g as zero. In order
to always respect the user-defined settings, we need to check our input
to see if an "-XX:MaxDirectMemorySize" value has been passed.

* Always warn for Windows/jdk8 ergo issue

Even if a user has set MaxDirectMemorySize, they aren't future-proof for
this JDK bug. With this change, we issue a general warning for the
windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is
unset.
williamrandolph added a commit that referenced this pull request Nov 12, 2019
* Warn when MaxDirectMemorySize may be incorrect (Windows/JDK8 only issue) (#48365)

Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. 
On JDK 8, there is a system-dependent bug where memory sizes are cast to 
32-bit integers. On affected systems (namely, Windows), when 1/4 of physical
memory is more than the maximum integer value, the output of PrintFlagsFinal
will be inaccurate. In the pathological case, where the max heap size would
be a multiple of 4g, the test will fail.

The practical effect of this bug, beyond test failures, is that we may set
MaxDirectMemorySize to an incorrect value on Windows. This commit adds a
warning about this situation during startup.

* Don't drop user's MaxDirectMemorySize flag on jdk8/windows (#48657)

* Always pass user-specified MaxDirectMemorySize

We had been testing whether a user had passed a value for
MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal
-version". If MaxDirectMemorySize equals zero, we set it to half of max
heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly
truncates values over 4g and returns multiples of 4g as zero. In order
to always respect the user-defined settings, we need to check our input
to see if an "-XX:MaxDirectMemorySize" value has been passed.

* Always warn for Windows/jdk8 ergo issue

Even if a user has set MaxDirectMemorySize, they aren't future-proof for
this JDK bug. With this change, we issue a general warning for the
windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is
unset.
williamrandolph added a commit that referenced this pull request Nov 15, 2019
…8/Windows (#49043)

* Warn when MaxDirectMemorySize may be incorrect (Windows/JDK8 only issue) (#48365)

Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output.
On JDK 8, there is a system-dependent bug where memory sizes are cast to
32-bit integers. On affected systems (namely, Windows), when 1/4 of physical
memory is more than the maximum integer value, the output of PrintFlagsFinal
will be inaccurate. In the pathological case, where the max heap size would
be a multiple of 4g, the test will fail.

The practical effect of this bug, beyond test failures, is that we may set
MaxDirectMemorySize to an incorrect value on Windows. This commit adds a
warning about this situation during startup.

On 7.4, we also warn for io.netty.allocator.type.

* Don't drop user's MaxDirectMemorySize flag on jdk8/windows (#48657)

* Always pass user-specified MaxDirectMemorySize

We had been testing whether a user had passed a value for
MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal
-version". If MaxDirectMemorySize equals zero, we set it to half of max
heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly
truncates values over 4g and returns multiples of 4g as zero. In order
to always respect the user-defined settings, we need to check our input
to see if an "-XX:MaxDirectMemorySize" value has been passed.

* Always warn for Windows/jdk8 ergo issue

Even if a user has set MaxDirectMemorySize, they aren't future-proof for
this JDK bug. With this change, we issue a general warning for the
windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is
unset.

* Adjust warning for io.netty.allocator.type
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.

4 participants