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.

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

Fix Edge-Case Threading Bug in TransportMountSearchableSnapshotAction #73196

Merged

Conversation

original-brownbear
Copy link
Member

The callback to loading the repository-data may not run on generic in the uncached case
because of the repo data deduplication logic.
The same issue was fixed for the snapshot status API in #68023

The callback to loading the repository-data may not run on generic in the uncached case
because of the repo data deduplication logic.
The same issue was fixed for the snapshot status API in elastic#68023
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 18, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@@ -280,6 +280,6 @@ protected void masterOperation(
.snapshotUuid(snapshotId.getUUID()),
listener
);
}, listener::onFailure);
}, listener::onFailure), threadPool.generic(), null);
Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in #73172 we could use the new snapshot meta pool here to limit concurrency of this operation. I think it makes sense to do that.

@DaveCTurner
Copy link
Contributor

Ugh this corner case is so trappy. Yeah I know it's in the docs, but clearly we don't read them enough. Maybe always forking would be better?

Anyway yes I'd prefer to merge #73172 and go straight onto the new pool rather than merge 38168a6 as it stands.

@original-brownbear
Copy link
Member Author

Maybe always forking would be better?

You mean in the repository? The annoying thing about that is that it may cause a number of needless context switches where we don't actually have to fork off. But maybe with the new pool that's ok and we could always switch to it even if it causes some needless switches.

For now I just made this thing fork to the new pool though.

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

@original-brownbear
Copy link
Member Author

Thanks David!

@original-brownbear original-brownbear merged commit 814839e into elastic:master May 18, 2021
@original-brownbear original-brownbear deleted the mount-snapshot-fix branch May 18, 2021 14:19
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jun 29, 2021
…elastic#73196)

The callback to loading the repository-data may not run on generic in the uncached case
because of the repo data deduplication logic.
The same issue was fixed for the snapshot status API in elastic#68023
original-brownbear added a commit that referenced this pull request Jun 29, 2021
…#73196) (#74695)

The callback to loading the repository-data may not run on generic in the uncached case
because of the repo data deduplication logic.
The same issue was fixed for the snapshot status API in #68023
@original-brownbear original-brownbear restored the mount-snapshot-fix branch April 18, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants