Skip to content

Use ThreadedActionListener in ListenableFuture #94367

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

Conversation

DaveCTurner
Copy link
Contributor

Following #94363 we can use ThreadedActionListener to represent the forking that happens within ListenableFuture, rather than tracking the executor and listener as separate components of a Tuple.

Following elastic#94363 we can use `ThreadedActionListener` to represent the
forking that happens within `ListenableFuture`, rather than tracking the
executor and listener as separate components of a Tuple.
@DaveCTurner DaveCTurner added :Core/Infra/Core Core issues without another label >refactoring v8.8.0 labels Mar 7, 2023
@DaveCTurner DaveCTurner requested a review from thecoop March 7, 2023 13:29
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 7, 2023
@elasticsearchmachine
Copy link
Collaborator

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

if (done) {
return false;
}
if (threadContext != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: these operations could be done outside the lock. Not sure if there's enough contention here to make any difference though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there isn't all that much contention here in practice, and it's been like this since it was introduced 4 years ago in #37327.

Copy link
Member

@thecoop thecoop left a comment

Choose a reason for hiding this comment

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

LGTM, one minor ordering comment

@DaveCTurner
Copy link
Contributor Author

@elasticmachine update branch

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 7, 2023
@DaveCTurner DaveCTurner merged commit 3b9a21f into elastic:main Mar 7, 2023
@DaveCTurner DaveCTurner deleted the 2023-03-07-ListenableFuture-using-ThreadedActionListener branch March 7, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants