-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
JvmErgonomicsTests.testExtractValidHeapSizeNoOptionPresent fails on 7.4 #47384
Comments
Pinging @elastic/es-core-infra |
Every single Windows 2012r2 build on the 7.4 branch this month has failed with this same error: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+7.4+multijob-windows-compatibility/os=windows-2012-r2/ |
The fact that this hasn't been failing in 7.x doesn't mean it's not a problem in 7.x. The 7.x builds all fail because of the minio problem - see #42829 - and the test report shows that There are no differences in either It's working in master though: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-windows-compatibility/os=windows-2012-r2/203/testReport/org.elasticsearch.tools.launchers/JvmErgonomicsTests/ |
As predicted, here's the failure on |
It looks like this is a JDK8 issue. The default value for MaxHeapSize is Using Powershell, the results come out like this:
That is the correct value, i.e., 3g => 3221225472 bytes. But:
That's not right! Or rather, Unfortunately, our Windows tests are executing on instances with 96 GB RAM, meaning the 24GB max heap size is getting truncated to 0 in the output that we're parsing. JDK 9 and beyond don't have this problem, and the master branch uses JDK 11 for the runtime, so that's why we aren't seeing this issue on master. As for the fix — is there a way to make this test meaningful, given that there's a bug in the JDK 8 output? Also, is this a potential problem on Linux, or is it Windows-specific? I'll keep digging on this. |
The behavior turns out to be system-dependent — "uintx" holds the correct values on my Mac, but not on my Windows test machines. The JDK bug report flagging this issue is here: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8074459 Since the problem behavior is in the JDK and not in our code, I think I will put out a PR that skips this test only on affected systems. |
On second thought, this bug might have some consequences for our ergonomic settings on affected machines. I'll figure out what the worst case would be and loop back for discussion. |
Note that we closed a related ticket by muting the tests on Windows: #44669 (comment) |
Here's what I've found. Two properties depend on the parsed value of
In both cases, the workaround is for the user to define the value or upgrade to a JDK more recent than 8. Since we have workarounds, my main question is how to put this information in some place where support or our users can find it. |
Would it be too harsh to bail telling the user about the bug and advising a heap size that doesn't fit the problematic pattern ? If my understanding is correct, just adding a byte to the heap would fix this. That way we wouldn't create any burden on users and support finding it at a slight inconvenience when everything lines up for the bug to reproduce, but would also save some trouble. |
A PR has been merged that removes the ergonomic |
Just like elastic#48329 (and using the changes) in that PR we can run into a concurrent repo modification that we will throw on and must retry until consistent handling of this situation is implemented. Closes elastic#47384
It fails with:
What's strange is that this did not print any reproduction line, so I wasn't able to try the exact test.
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+7.4+multijob-windows-compatibility/os=windows-2019/64/consoleFull
https://gradle-enterprise.elastic.co/s/mm3dqwj56d4aa/console-log
The text was updated successfully, but these errors were encountered: