Skip to content

Fix Edge-Case Threading Bug in TransportMountSearchableSnapshotAction#73196

Merged
original-brownbear merged 4 commits intoelastic:masterfrom
original-brownbear:mount-snapshot-fix
May 18, 2021
Merged

Fix Edge-Case Threading Bug in TransportMountSearchableSnapshotAction#73196
original-brownbear merged 4 commits intoelastic:masterfrom
original-brownbear:mount-snapshot-fix

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

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 #68023
@original-brownbear original-brownbear added >bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.14.0 labels May 18, 2021
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team. label May 18, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

listener
);
}, listener::onFailure);
}, listener::onFailure), threadPool.generic(), null);
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Member

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
Copy Markdown
Contributor 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
Copy Markdown
Member

@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
Copy Markdown
Contributor 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 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/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team. v7.14.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants