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.

Sign up for GitHub

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

Jump to bottom

Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug #104347

Merged
merged 11 commits into from
Jan 16, 2024
Merged

Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug #104347

merged 11 commits into from
Jan 16, 2024

Conversation

ChrisHegarty
Copy link
Contributor

@ChrisHegarty ChrisHegarty commented Jan 13, 2024 •
edited by ldematte

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue [1]. Without this change tasks submitted to the executor service vanish!

The underlying issue is a JDK bug (or at least an unexpected change in behaviour), regardless of whether or not that JDK bug gets fixed, the next scheduled JDK update release, 21.0.2, has the issue. So for ES to run on this JDK release we'll need the workaround proposed in this PR.

relates #103963

Closes #104067
Closes #104103

[1] https://bugs.openjdk.org/browse/JDK-8323659

@ChrisHegarty ChrisHegarty added :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v7.17.17 v8.12.1 v8.11.5 labels Jan 13, 2024
@elasticsearchmachine
Copy link
Collaborator

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

ChrisHegarty
ChrisHegarty commented Jan 13, 2024
View reviewed changes
server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java Outdated
@Override
public void put(E e) {
boolean added = super.offer(e);
assert added;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works because LTQ is unbounded, offer will always succeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 maybe add this as a code comment

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

ldematte
ldematte approved these changes Jan 15, 2024
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

Since this is delicate, you probably want to wait for approval from the people you listed. Anyway thanks to the linked docs I was able to figure out why the change is needed, and it looks good to me.

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

DaveCTurner
DaveCTurner reviewed Jan 16, 2024
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Looks good to me too, I left a few superficial suggestions/comments

server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/util/concurrent/EsExecutorsTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/util/concurrent/EsExecutorsTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/util/concurrent/EsExecutorsTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/util/concurrent/EsExecutorsTests.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java Outdated
@Override
public void put(E e) {
boolean added = super.offer(e);
assert added;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 maybe add this as a code comment

ChrisHegarty and others added 4 commits January 16, 2024 09:50
@ChrisHegarty @DaveCTurner
Update server/src/test/java/org/elasticsearch/common/util/concurrent/…
0b87472 
…EsExecutorsTests.java

Co-authored-by: David Turner <david.turner@elastic.co>
@ChrisHegarty @DaveCTurner
Update server/src/test/java/org/elasticsearch/common/util/concurrent/…
6deae63 
…EsExecutorsTests.java

Co-authored-by: David Turner <david.turner@elastic.co>
@ChrisHegarty @DaveCTurner
Update server/src/test/java/org/elasticsearch/common/util/concurrent/…
6139d17 
…EsExecutorsTests.java

Co-authored-by: David Turner <david.turner@elastic.co>
@ChrisHegarty
review comments
1f2ed13
@ChrisHegarty
Copy link
Contributor Author

Thanks for the detailed review comments @DaveCTurner, I think that I've addressed all of them. Please take another look.

DaveCTurner
DaveCTurner approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

This was referenced Jan 16, 2024
Closed
Closed
rjernst
rjernst approved these changes Jan 16, 2024
View reviewed changes
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

@ChrisHegarty ChrisHegarty merged commit 81ec404 into elastic:main Jan 16, 2024
15 checks passed
@ChrisHegarty ChrisHegarty added the auto-backport Automatically create backport pull requests when merged label Jan 16, 2024
This was referenced Jan 16, 2024
Merged
Merged
ChrisHegarty added a commit to ChrisHegarty/elasticsearch that referenced this pull request Jan 16, 2024
@ChrisHegarty
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
cf463c1 
…elastic#104347)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
@ChrisHegarty ChrisHegarty mentioned this pull request Jan 16, 2024
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 7.17
✅ 8.12
✅ 8.11

ChrisHegarty added a commit to ChrisHegarty/elasticsearch that referenced this pull request Jan 16, 2024
@ChrisHegarty
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
7a65476 
…elastic#104347)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
ChrisHegarty added a commit to ChrisHegarty/elasticsearch that referenced this pull request Jan 16, 2024
@ChrisHegarty
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
d1dccb3 
…elastic#104347)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
costin pushed a commit to costin/elasticsearch that referenced this pull request Jan 17, 2024
@ChrisHegarty @costin
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
6192505 
…elastic#104347)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
jedrazb pushed a commit to jedrazb/elasticsearch that referenced this pull request Jan 17, 2024
@ChrisHegarty @jedrazb
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
a7855b5 
…elastic#104347)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
ChrisHegarty added a commit that referenced this pull request Jan 17, 2024
@ChrisHegarty
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
f2ef03a 
…#104347) (#104426)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
ChrisHegarty added a commit that referenced this pull request Jan 17, 2024
@ChrisHegarty
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (…
e98d634 
…#104347) (#104427)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
@ChrisHegarty ChrisHegarty mentioned this pull request Jan 17, 2024
Closed
elasticsearchmachine pushed a commit that referenced this pull request Jan 17, 2024
@ChrisHegarty
[7.17] Update ExecutorScalingQueue to workaround LinkedTransferQueue …
af3f8e5 
…JDK bug (#104347) (#104425)

* Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug (#104347)

This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.

* remove usage of var

* fix compilation
@reta reta mentioned this pull request Jan 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst approved these changes

@ldematte ldematte ldematte approved these changes

@DaveCTurner DaveCTurner DaveCTurner approved these changes

@mark-vieira mark-vieira Awaiting requested review from mark-vieira

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v7.17.17 v8.11.5 v8.12.1 v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] ClientYamlTestSuiteIT classMethod failing [CI] XPackRestIT classMethod failing
6 participants
@ChrisHegarty @elasticsearchmachine @rjernst @ldematte @DaveCTurner @elasticmachine