-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Update ExecutorScalingQueue to workaround LinkedTransferQueue JDK bug #104347
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
@Override | ||
public void put(E e) { | ||
boolean added = super.offer(e); | ||
assert added; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@elasticmachine update branch |
@elasticmachine update branch |
There was a problem hiding this 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.
@elasticmachine update branch |
There was a problem hiding this 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
@Override | ||
public void put(E e) { | ||
boolean added = super.offer(e); | ||
assert added; |
There was a problem hiding this comment.
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
…EsExecutorsTests.java Co-authored-by: David Turner <david.turner@elastic.co>
…EsExecutorsTests.java Co-authored-by: David Turner <david.turner@elastic.co>
…EsExecutorsTests.java Co-authored-by: David Turner <david.turner@elastic.co>
Thanks for the detailed review comments @DaveCTurner, I think that I've addressed all of them. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…elastic#104347) This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
…elastic#104347) This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
…elastic#104347) This commit adds a few overrides to ExecutorScalingQueue (subclass of LinkedTransferQueue) to workaround a JDK bug in LinkedTransferQueue.
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