Flatten Get Snapshots Response #74451

original-brownbear · 2021-06-22T20:01:53Z

This PR returns the get snapshots API to the 7.x format (and transport client behavior) and enhances it for requests that ask for multiple repositories.
The changes for requests that target multiple repositories are:

Add repository field to SnapshotInfo and REST response
Add failures map alongside snapshots list instead of returning just an exception response as done for single repo requests
Pagination now works across repositories instead of being per repository for multi-repository requests

closes #69108
closes #43462

…ots-response

original-brownbear · 2021-06-23T10:02:47Z

Jenkins run elasticsearch-ci/docs (git ssh timeout)

…ots-response

henningandersen

I have only been through the non-test file changes for now. Mainly one comment needs attention: error handling.

.../src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java

docs/reference/snapshot-restore/monitor-snapshot-restore.asciidoc

...src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponse.java

henningandersen · 2021-06-24T11:12:32Z

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java

@@ -160,12 +172,12 @@ private void getMultipleReposSnapshotInfo(
                size,
                order,
                groupedActionListener.delegateResponse((groupedListener, e) -> {
-                    if (e instanceof ElasticsearchException) {
-                        groupedListener.onResponse(GetSnapshotsResponse.Response.error(repoName, (ElasticsearchException) e));
+                    if (isMultiRepoRequest && e instanceof ElasticsearchException) {


I think this way of error handling is new compared to elsewhere. I wonder if you considered one of following options:

Respond with a failure if all repos failed (which would be the same as here in the single repo case). This is sort of similar to search.

Breakingly never respond failures, expect users to look into the failures map.

Have an option to fail the request or send back failures in response.

Do we have other similar cases in ES we can lean on in order to decide?

Respond with a failure if all repos failed (which would be the same as here in the single repo case). This is sort of similar to search.

That seems more complicated to work with than having the failures map? My thinking was to have the same behavior for all requests that target multiple repos. I'm somewhat indifferent here, making it look like search is nice, but in isolation I like this approach better I think.

Breakingly never respond failures, expect users to look into the failures map.

This is quite annoying to users perhaps (note that in 7.x we also have to think about the transport client behavior that would be changed by this). It may also be a troubling breaking change for Cloud? I'd rather not do this I think.

Have an option to fail the request or send back failures in response.

I don't think we have that kind of behavior anywhere else? Not sure it's worth the complexity for a very small number of use cases?

Do we have other similar cases in ES we can lean on in order to decide?

None come to mind for me that are similar enough but that doesn't mean there aren't any. I think I was in large part driven by not breaking the transport client API for users and cloud orchestration. In both cases changing the behavior for a single repo request seems risky?

We discussed this on another channel with following conclusion:

We will merge this PR as is.

We will follow-up with a PR to add the option to fail-fast or not, regardless of multi/single repo in regular breaking change style. The default in 7 will be fail fast, default in 8 will be always put it in array. Flag to be removed in 9.

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

original-brownbear · 2021-06-24T12:36:22Z

Thanks @henningandersen all points addressed now I think :)

elasticmachine · 2021-06-24T13:03:06Z

Pinging @elastic/clients-team (Team:Clients)

henningandersen

A few minor comments on the tests

henningandersen · 2021-06-24T12:25:35Z

client/rest-high-level/src/test/java/org/elasticsearch/client/SnapshotIT.java

+        assertThat(response.getSnapshots(), hasSize(2));
+        assertThat(response.getSnapshots().get(0).snapshotId().getName(), equalTo(snapshot1));
+        assertThat(response.getSnapshots().get(1).snapshotId().getName(), equalTo(snapshot2));
+        assertThat(response.getSnapshots().get(1).userMetadata(), equalTo(originalMetadata));


Should we also validate that snapshot1 has repo1 and snapshot2 has repo2?

Yes definitely, added that assertion :)

server/src/internalClusterTest/java/org/elasticsearch/snapshots/SnapshotStatusApisIT.java

henningandersen · 2021-06-24T12:49:52Z

...qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/slm/SnapshotLifecycleRestIT.java

-                .peek(m -> logger.info("--> responses: {}", m))
-                .map(m -> (List<Map<String, Object>>) m.get("snapshots"))
-                .peek(allReposSnapshots -> logger.info("--> all repository's snapshots: {}", allReposSnapshots))
-                .filter(allReposSnapshots -> allReposSnapshots.stream().anyMatch(repoHasSnapshot))


repoHasSnapshot is now unused. I think it makes sense that this predicate is still validated.

++ removed it and checked the snapshot name prefix manually below

henningandersen

LGTM

tlrx

LGTM 2

original-brownbear · 2021-06-24T14:58:16Z

Thanks so much Henning + Tanguy!

This PR returns the get snapshots API to the 7.x format (and transport client behavior) and enhances it for requests that ask for multiple repositories. The changes for requests that target multiple repositories are: * Add `repository` field to `SnapshotInfo` and REST response * Add `failures` map alongside `snapshots` list instead of returning just an exception response as done for single repo requests * Pagination now works across repositories instead of being per repository for multi-repository requests closes elastic#69108 closes elastic#43462

) 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)

The group-by-repository response format is gone as a result of elastic#74451 => cleaning up its last remnants from REST tests.

The group-by-repository response format is gone as a result of #74451 => cleaning up its last remnants from REST tests.

original-brownbear added 4 commits June 22, 2021 19:10

step

21665ae

Merge remote-tracking branch 'elastic/master' into flatten-get-snapsh…

3b094da

…ots-response

fix it

e7b70ac

works

fff9899

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

original-brownbear added 2 commits June 23, 2021 09:04

Merge remote-tracking branch 'elastic/master' into flatten-get-snapsh…

75bf116

…ots-response

global sort order

c4cb98f

original-brownbear mentioned this pull request Jun 23, 2021

Support v7 response format for get-snapshots API #69108

Closed

original-brownbear added 5 commits June 23, 2021 10:52

bwc and test

6c59f0d

Merge remote-tracking branch 'elastic/master' into flatten-get-snapsh…

38d4780

…ots-response

Merge remote-tracking branch 'elastic/master' into flatten-get-snapsh…

588180e

…ots-response

fix test

902c259

even moar test adjustments

52139d7

original-brownbear added 8 commits June 23, 2021 12:14

reenable tests

2e1c9e9

reactivate more

d1529eb

so many tests

3b4bf39

woerks

719581c

fix all kinds of tests

731915a

and another one ...'

fb687f1

finally?

3f3c525

Merge remote-tracking branch 'elastic/master' into flatten-get-snapsh…

b6b218f

…ots-response

original-brownbear mentioned this pull request Jun 23, 2021

Improve Snapshot Repository Scalability #74350

Closed

16 tasks

original-brownbear added 5 commits June 23, 2021 14:43

fix more

0393b0e

fix more

49a66e0

fix

b8a36af

fix

f2da1ba

one more

0a0fe71

original-brownbear added 2 commits June 24, 2021 13:40

include repository in responses by default

bcade7d

default include snapshot name, add test for it

09b49f6

henningandersen reviewed Jun 24, 2021

View reviewed changes

CR: comments

617ad0f

original-brownbear requested a review from henningandersen June 24, 2021 12:36

sethmlarson added the Team:Clients Meta label for clients team label Jun 24, 2021

henningandersen reviewed Jun 24, 2021

View reviewed changes

henningandersen approved these changes Jun 24, 2021

View reviewed changes

original-brownbear added 2 commits June 24, 2021 15:59

CR: test changes

14f88f3

typo

63b19e3

tlrx approved these changes Jun 24, 2021

View reviewed changes

original-brownbear merged commit cbf48e0 into elastic:master Jun 24, 2021

original-brownbear deleted the flatten-get-snapshots-response branch June 24, 2021 14:58

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

alisonelizabeth mentioned this pull request Jun 24, 2021

[Snapshot + Restore] Set snapshots response size limit elastic/kibana#103331

Merged

original-brownbear mentioned this pull request Jun 28, 2021

Introduce Next Field in Paginated GetSnapshots Response #74236

Merged

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

original-brownbear mentioned this pull request Jun 30, 2021

Remove Outdated GetSnapshots BwC Handling in Rest Tests #74734

Merged

original-brownbear added a commit that referenced this pull request Jun 30, 2021

Remove Outdated GetSnapshots BwC Handling in Rest Tests (#74734)

eca5735

The group-by-repository response format is gone as a result of #74451 => cleaning up its last remnants from REST tests.

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

joegallo mentioned this pull request Mar 31, 2022

Prevent deletion of snapshots that are used by snapshot backed indices #73821

Closed

original-brownbear restored the flatten-get-snapshots-response branch April 18, 2023 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatten Get Snapshots Response #74451

Flatten Get Snapshots Response #74451

original-brownbear commented Jun 22, 2021 •

edited

Loading

original-brownbear commented Jun 23, 2021

henningandersen left a comment

henningandersen Jun 24, 2021

original-brownbear Jun 24, 2021

henningandersen Jun 24, 2021

original-brownbear commented Jun 24, 2021

elasticmachine commented Jun 24, 2021

henningandersen left a comment

henningandersen Jun 24, 2021

original-brownbear Jun 24, 2021

henningandersen Jun 24, 2021

original-brownbear Jun 24, 2021

henningandersen left a comment

tlrx left a comment

original-brownbear commented Jun 24, 2021

Flatten Get Snapshots Response #74451

Flatten Get Snapshots Response #74451

Conversation

original-brownbear commented Jun 22, 2021 • edited Loading

original-brownbear commented Jun 23, 2021

henningandersen left a comment

Choose a reason for hiding this comment

henningandersen Jun 24, 2021

Choose a reason for hiding this comment

original-brownbear Jun 24, 2021

Choose a reason for hiding this comment

henningandersen Jun 24, 2021

Choose a reason for hiding this comment

original-brownbear commented Jun 24, 2021

elasticmachine commented Jun 24, 2021

henningandersen left a comment

Choose a reason for hiding this comment

henningandersen Jun 24, 2021

Choose a reason for hiding this comment

original-brownbear Jun 24, 2021

Choose a reason for hiding this comment

henningandersen Jun 24, 2021

Choose a reason for hiding this comment

original-brownbear Jun 24, 2021

Choose a reason for hiding this comment

henningandersen 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 24, 2021

original-brownbear commented Jun 22, 2021 •

edited

Loading