Add Bulk Fetch SnapshotInfo API to Repository #73570

original-brownbear · 2021-05-31T16:29:28Z

This PR refactors the Repository API for fetching SnapshotInfo to enabled implementations to optimize for bulk fetching multiple SnapshotInfo at once. This is a requirement for making use of a more efficient repository format that does not require loading individual blobs per snapshot to fetch a snapshot listing. Also, by enabling consuming SnapshotInfo as they are fetched on the snapshot meta thread this allows for some more memory efficient usage of snapshot listing.
Also, this commit makes use of the new API to make the snapshot status API run a little more parallel if fetching multiple snapshots (though there's additional improvements possible+useful here as far as fetching shard level metadata in parallel).

elasticmachine · 2021-05-31T16:29:31Z

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

…pshot-info-api

With work to make repo APIs more async incoming in elastic#73570 we need a non-blocking way to run this check. This adds that async check and removes the need to manually pass executors around as well.

With work to make repo APIs more async incoming in #73570 we need a non-blocking way to run this check. This adds that async check and removes the need to manually pass executors around as well.

server/src/main/java/org/elasticsearch/repositories/GetSnapshotInfoContext.java

tlrx · 2021-06-14T08:17:00Z

.../org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java

-                            (endTime == 0 ? threadPool.absoluteTimeInMillis() : endTime) - startTime
-                        )
-                    );
+                if (snapshotsInProgress.snapshot(new Snapshot(repositoryName, snapshotId)) == null) {


I'm not sure to understand this: we found a snapshot with a matching name in the repository data but we did not found it before in the in progress snapshots, how could it be in snapshotsInProgress?

Well spotted :) I think this is an impossible race to run into these days. I'll leave it as is here for now and will open a PR to clean this up from master if possible separately today :)

tlrx · 2021-06-14T08:25:32Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+    public void getSnapshotInfo(GetSnapshotInfoContext context) {
+        // put snapshot info downloads into a task queue instead of pushing them all into the queue to not completely monopolize the
+        // snapshot meta pool for a single request
+        final int workers = Math.min(threadPool.info(ThreadPool.Names.SNAPSHOT_META).getMax(), context.snapshotIds().size());


Maybe check context.done() and context.isCancelled() first here too and return directly? This would avoid to queue runnables in the snapshot meta thread pool for nothing (yet it is unlikely to happen)

I figured that wasn't really worth it since we haven't forked yet. The chance of a concurrent cancellation is almost 0 and the cannot have been failed yet concurrently ever I think.

tlrx · 2021-06-14T08:29:52Z

.../org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java

+            } else {
+                final List<SnapshotStatus> threadSafeBuilder = Collections.synchronizedList(builder);
+                repositoriesService.repository(repositoryName)
+                    .getSnapshotInfo(new GetSnapshotInfoContext(snapshotIdsToLoad, true, () -> false, (context, snapshotInfo) -> {


Maybe pass task::isCancelled instead of () -> false ? The task cancellation is verified also in snapshotShards() few lines after but maybe we could be more explicit?

++ right, this PR was opened before the status action became cancellable, will fix :)

tlrx · 2021-06-14T08:34:48Z

.../org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java

+                            shardStatuses = snapshotShards(repositoryName, repositoryData, task, snapshotInfo);
+                        } catch (Exception e) {
+                            // stops all further fetches of snapshotInfo since context is fail-fast
+                            context.onFailure(e);


I wonder if we could turn the GetSnapshotInfoContext's biconsumer into a checked consumer and let `GetSnapshotInfoContext catch exceptions and fail the context? (see other comment below)

tlrx · 2021-06-14T08:36:16Z

server/src/main/java/org/elasticsearch/repositories/GetSnapshotInfoContext.java

+     * {@link BiConsumer} invoked for each {@link SnapshotInfo} that is fetched with this instance and the {@code SnapshotInfo} as
+     * arguments.
+     */
+    private final BiConsumer<GetSnapshotInfoContext, SnapshotInfo> onSnapshotInfo;


I wonder if this could be a CheckedConsumer<SnapshotInfo, IOException> onSnapshotInfo and handle failure in case of exception directly in this class rather than the consumer holds a reference to the context in order to be able to fail it.

(Unless this is required by an upcoming change?)

(Unless this is required by an upcoming change?)

Yes I think I'd like the flexibility for upcoming changes. The fact that we currently fetch the shard level metadata on the thread that resolves the snapshot info listener is not supposed to stick around as a solution so we need this mechanism to fail a forking callback here.

tlrx · 2021-06-14T08:37:44Z

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

+            } catch (NoSuchFileException ex) {
+                failure = new SnapshotMissingException(metadata.name(), snapshotId, ex);
+            } catch (IOException | NotXContentException ex) {
+                failure = new SnapshotException(metadata.name(), snapshotId, "failed to get snapshots", ex);


Suggested change

failure = new SnapshotException(metadata.name(), snapshotId, "failed to get snapshots", ex);

failure = new SnapshotException(metadata.name(), snapshotId, "failed to get snapshot info for " + snapshotId, ex);

No need to add the snapshot id to the message IMO, we already have it as a field on the snapshot exception and that will be serialized. But it should be singular here in the message I think :)

…pshot-info-api

DaveCTurner

Not much to add over Tanguy's thorough review, just a few comments on habitability.

DaveCTurner · 2021-06-14T12:08:31Z

server/src/main/java/org/elasticsearch/repositories/GetSnapshotInfoContext.java

+        BiConsumer<GetSnapshotInfoContext, SnapshotInfo> onSnapshotInfo,
+        ActionListener<Void> listener
+    ) {
+        assert snapshotIds.isEmpty() == false : "no snapshot ids to fetch given";


IMO we should either forbid this in production too (e.g. throw an IllegalArgumentException) or else just allow it and deal with the n=0 case ourselves. I'd rather the latter I think but either way is ok.

I went with throwing for now. I think we shouldn't create the case where we pass an empty list to this API, just like we don't allow a 0 sized GroupedActionListener, otherwise we'd have to resolve the listener in the constructor right away I guess, which I find confusing.

server/src/main/java/org/elasticsearch/repositories/GetSnapshotInfoContext.java

server/src/main/java/org/elasticsearch/repositories/Repository.java

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

server/src/test/java/org/elasticsearch/repositories/RepositoriesServiceTests.java

test/framework/src/main/java/org/elasticsearch/index/shard/RestoreOnlyRepository.java

…pshot-info-api

original-brownbear · 2021-06-14T14:14:56Z

Thanks Tanguy + David! I think I addressed all comments now and this should be good for another round :)

DaveCTurner · 2021-06-14T14:30:18Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRepository.java

@@ -182,8 +185,15 @@ public SnapshotInfo getSnapshotInfo(SnapshotId snapshotId) {
        ArrayList<String> indices = new ArrayList<>(indicesMap.size());
        indicesMap.keysIt().forEachRemaining(indices::add);

-        return new SnapshotInfo(snapshotId, indices, new ArrayList<>(metadata.dataStreams().keySet()), Collections.emptyList(),
-            response.getState().getNodes().getMaxNodeVersion(), SnapshotState.SUCCESS
+        context.onResponse(


Oh oops I just saw this. Think we need to take steps to put this on the SNAPSHOT_META thread now.

timing #73570 (comment) :D on it shortly

fixed in 94f44ed now

original-brownbear · 2021-06-14T14:30:22Z

Ah damn the new assertion breaks the CCR repo which resolves the listener straight away, will fix in ~30 minutes

DaveCTurner

LGTM; I haven't reviewed the changes in TransportSnapshotsStatusAction terribly deeply, they look roughly ok but I'd have to spend more time on them to be sure and it looks like Tanguy has already done that.

tlrx

LGTM, thanks for the extra iterations

original-brownbear · 2021-06-14T17:17:35Z

Thanks Tanguy + David!

Pagination and snapshots for get snapshots API, build on top of the current implementation to enable work that needs this API for testing. A follow-up will leverage the changes to make things more efficient via pagination. Relates #73570 which does part of the under-the-hood changes required to efficiently implement this API on the repository layer.

) Backport of the recently introduced snapshot pagination and scalability improvements listed below. Merged as a single backport because the `7.x` and master snapshot status API logic had massively diverged between master and 7.x. With the work in the below PRs, the logic in master and 7.x once again has been aligned very closely again. #72842 #73172 #73199 #73570 #73952 #74236 #74451 (this one is only partly applicable as it was mainly a change to master to align `master` and `7.x` branches)

worksish

4fb132b

original-brownbear added WIP :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >refactoring labels May 31, 2021

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label May 31, 2021

original-brownbear marked this pull request as draft May 31, 2021 16:29

original-brownbear added 11 commits May 31, 2021 18:54

fix things + docs

0b2b29f

fix concurrency

4d15949

adjust

d6f3e3c

snapshot status API fetch logic

0ef6f1b

some additonal docs

c02244a

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

9b27b0b

…pshot-info-api

fix todo

24de437

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

97a7082

…pshot-info-api

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

749f882

…pshot-info-api

cleanup

bc4515e

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

2b082fd

…pshot-info-api

original-brownbear changed the title ~~[WIP] Add Bulk Fetch SnapshotInfo API to Repository~~ Add Bulk Fetch SnapshotInfo API to Repository Jun 2, 2021

original-brownbear marked this pull request as ready for review June 2, 2021 12:54

original-brownbear added v7.14.0 v8.0.0 WIP and removed WIP labels Jun 2, 2021

original-brownbear marked this pull request as draft June 2, 2021 13:57

original-brownbear added 3 commits June 2, 2021 15:58

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

f4355fa

…pshot-info-api

tests fixed

854f443

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

7c3c422

…pshot-info-api

original-brownbear removed the WIP label Jun 7, 2021

original-brownbear marked this pull request as ready for review June 7, 2021 08:17

original-brownbear mentioned this pull request Jun 13, 2021

Simplify Blobstore Consistency Check in Tests (#73992) #74045

Merged

tlrx reviewed Jun 14, 2021

View reviewed changes

original-brownbear added 2 commits June 14, 2021 13:38

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

b38e4dc

…pshot-info-api

CR: comments

fe4e0ce

DaveCTurner reviewed Jun 14, 2021

View reviewed changes

original-brownbear added 3 commits June 14, 2021 15:13

CR: comments

b874ef5

Merge remote-tracking branch 'elastic/master' into more-efficient-sna…

54e9dea

…pshot-info-api

docs and assertions

d875e8c

original-brownbear requested review from tlrx and DaveCTurner June 14, 2021 14:14

DaveCTurner reviewed Jun 14, 2021

View reviewed changes

DaveCTurner approved these changes Jun 14, 2021

View reviewed changes

fix ccr

94f44ed

tlrx approved these changes Jun 14, 2021

View reviewed changes

original-brownbear merged commit dbb626a into elastic:master Jun 14, 2021

original-brownbear deleted the more-efficient-snapshot-info-api branch June 14, 2021 17:17

original-brownbear added the backport pending label Jun 14, 2021

original-brownbear mentioned this pull request Jun 21, 2021

Improve Snapshot Repository Scalability #74350

Closed

16 tasks

original-brownbear mentioned this pull request Jun 29, 2021

Snapshot Pagination and Scalability Improvements Backport to 7.x #74676

Merged

original-brownbear removed the backport pending label Jun 29, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

original-brownbear restored the more-efficient-snapshot-info-api branch April 18, 2023 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Bulk Fetch SnapshotInfo API to Repository #73570

Add Bulk Fetch SnapshotInfo API to Repository #73570

original-brownbear commented May 31, 2021 •

edited

Loading

elasticmachine commented May 31, 2021

tlrx Jun 14, 2021

original-brownbear Jun 14, 2021

tlrx Jun 14, 2021

original-brownbear Jun 14, 2021

tlrx Jun 14, 2021

original-brownbear Jun 14, 2021

tlrx Jun 14, 2021

tlrx Jun 14, 2021

original-brownbear Jun 14, 2021

tlrx Jun 14, 2021

original-brownbear Jun 14, 2021

DaveCTurner left a comment

DaveCTurner Jun 14, 2021

original-brownbear Jun 14, 2021

original-brownbear commented Jun 14, 2021

DaveCTurner Jun 14, 2021

original-brownbear Jun 14, 2021

original-brownbear Jun 14, 2021

original-brownbear commented Jun 14, 2021

DaveCTurner left a comment

tlrx left a comment

original-brownbear commented Jun 14, 2021

	failure = new SnapshotException(metadata.name(), snapshotId, "failed to get snapshots", ex);
	failure = new SnapshotException(metadata.name(), snapshotId, "failed to get snapshot info for " + snapshotId, ex);

Add Bulk Fetch SnapshotInfo API to Repository #73570

Add Bulk Fetch SnapshotInfo API to Repository #73570

Conversation

original-brownbear commented May 31, 2021 • edited Loading

elasticmachine commented May 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Jun 14, 2021

DaveCTurner left a comment

Choose a reason for hiding this comment

tlrx left a comment

Choose a reason for hiding this comment

original-brownbear commented Jun 14, 2021

original-brownbear commented May 31, 2021 •

edited

Loading