-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Revert primary permits changes and add hook #120398
Revert primary permits changes and add hook #120398
Conversation
af0481e
to
bcd04fc
Compare
We would like to avoid using directly primary permits for hollow shards. So we revert relevant changes, and add a hook into the function that gets a primary permit with the purpose of a plugin being able to extend the behavior. Relates ES-10537
bcd04fc
to
275f5c0
Compare
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
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.
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. Looks great, I left a couple of small comments.
); | ||
|
||
ActionListener<Releasable> onPermitAcquiredWrapped = onPermitAcquired.delegateFailureAndWrap((delegate, releasable) -> { | ||
final ActionListener<Releasable> wrappedListener = indexShardOperationPermits.wrapContextPreservingActionListener( |
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.
do we need to do the context preserving in all cases? I don't think so? i.e. during normal operations
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.
@fcofdez I'm not familiar with what operations need the context. I saw it in IndexShardOperationPermits.java and assumed it's needed here as well.
Note that before this PR, the IndexShardOperationPermits.java was the one potentially triggering the final listener of the operation. However, with this PR, we potentially add another blocking step in the plugin (the additional RefCountingListener here). So we can't be sure whatever thread finally triggers the RefCountingListener has the context, thus I elected to made sure we preserve the context as well to "replicate" the original behavior of IndexShardOperationPermits.java.
After this explanation, do you believe we can safely skip the context?
We would like to avoid using directly primary permits for hollow shards. So we revert relevant changes, and add a hook into the function that gets a primary permit with the purpose of a plugin being able to extend the behavior.
Relates ES-10537