Introduce Next Field in Paginated GetSnapshots Response #74236

original-brownbear · 2021-06-17T12:41:35Z

Follow up to #73952 adding documentation for the after query parameter
and the related next response field.

Follow up to elastic#73952 adding documentation for the `after` query parameter and the related `next` response field.

elasticmachine · 2021-06-17T12:41:38Z

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

original-brownbear · 2021-06-17T15:03:32Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

+          }
+        }
+      ],
+      "next": "snapshot_2,snapshot_2"


I wonder if we should just not allow pagination when doing a query across multiple repos. The current approach of doing pagination per repo becomes really confusing when multiple next values are returned but only one after can be set. Maybe this doesn't matter too much in the short-run as it's an 8.x only issue anyway but for `8 we should have something nicer?

Hmm that does seem suboptimal, I think it would be useful to have pagination even if multiple repos are involved. Why do we need to return multiple next values? Since we're using the same sort order for all of them can we not use the minimum of the next values? Relatedly, does this mean that the limit applies to each repository separately rather than across all of them?

Relatedly, does this mean that the limit applies to each repository separately rather than across all of them?

Yes, currently that's how it's implemented (mainly because of the focus on getting this out the door for 7.x where this is a non-concern because we only work with a single repo at a time and I didn't want to create different logic for master and 7.x for now to keep the backport reasonably simple).

Since we're using the same sort order for all of them can we not use the minimum of the next values?
Why do we need to return multiple next values?

We can implement pagination globally across all repos but then the logic for master will diverge a lot from 7.x which I'd rather not do right now. If we don't do that though, I don't see an alternative to either forbidding pagination with multiple repos in the query or having multiple next values?

Ok, forbidding it seems the most future-proof answer today, it's easy to become more permissive in future but much harder to change something that works into something else that works differently.

I think it'd be better for the next field to be at the top level of the response tho rather than having it inside the (single) snapshot object.

The problems/confusion here has been resolved last week in #74451. We now have just a single next field at the top level and snapshots are paginated across repositories :)

…pshots

original-brownbear · 2021-06-28T11:57:39Z

@DaveCTurner @henningandersen sorry for the noise in this one, now that #74451 has been merged this one should be good to review. I added the next parameter at the top level now and it works across repositories (admittedly not yet in a super efficient manner but the API should behave well :)).

henningandersen

Left a few comments, looking good otherwise.

henningandersen · 2021-06-28T12:09:09Z

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

+            final String[] parts = param.split(",");
+            if (parts.length != 2) {
+                throw new IllegalArgumentException(
+                    "after param must be of the form ${sort_value},${snapshot_name} but was [" + param + "]"


Should we not include the repo as a tiebreaker too? I.e., the format is sort_value,repo_name,snapshot_name?

qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java

henningandersen · 2021-06-28T12:20:09Z

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

        this.snapshots = List.copyOf(snapshots);
        this.failures = failures == null ? Map.of() : Map.copyOf(failures);
+        this.next = next;
    }

    public GetSnapshotsResponse(StreamInput in) throws IOException {
        this.snapshots = in.readList(SnapshotInfo::readFrom);
        if (in.getVersion().onOrAfter(GetSnapshotsRequest.MULTIPLE_REPOSITORIES_SUPPORT_ADDED)) {


I take it you will backport both this and the multiple repo PR in one go then?

Depends on whether this goes in today I guess with FF incoming :) If not I'll adjust the PR and backport separately.

henningandersen · 2021-06-28T12:24:13Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

-NOTE: The pagination parameters `size`, `order`, and `sort` are not supported when using `verbose=false` and the sort order for
+`after`::
+(Optional, string)
+Offset identifier to start pagination from as returned by the `next` field in the response body.


I think it is worth spending a few lines explaining that the underlying list can change and how pagination works in case of deletes and additions during pagination (i.e., you are sure to not miss any that were in the list originally and have not been deleted during pagination and you may see snapshots added during pagination).

++ added a few lines on this

henningandersen · 2021-06-28T12:27:41Z

...est/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponseTests.java

@@ -119,7 +119,7 @@ private GetSnapshotsResponse createTestInstance() {
            failures.put(repository, new ElasticsearchException(randomAlphaOfLength(10)));
        }

-        return new GetSnapshotsResponse(responses, failures);
+        return new GetSnapshotsResponse(responses, failures, null);


I think we sometimes need to add a random next value here? Also, assertEqualInstances should be updated.

henningandersen · 2021-06-28T12:29:25Z

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

    }

    private final List<SnapshotInfo> snapshots;

    private final Map<String, ElasticsearchException> failures;

-    public GetSnapshotsResponse(List<SnapshotInfo> snapshots, Map<String, ElasticsearchException> failures) {
+    @Nullable
+    private final String next;


I think we should update equals and hashCode to account for the new field too.

DaveCTurner

I'm still reviewing this properly but had one comment straight away regarding encoding.

DaveCTurner · 2021-06-28T12:35:16Z

docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc

+
+[source,console]
+----
+GET /_snapshot/my_repository/snapshot*?size=2&sort=name&after=snapshot_2,snapshot_2


I worry we will have users running into encoding problems with this API, since we permit characters like & and % in snapshot names and the effects of failing to encode this parameter properly might be pretty subtle. Could we base64-encode this parameter to avoid all that?

++ will do :)

…pshots

original-brownbear · 2021-06-28T14:21:07Z

Jenkins run elasticsearch-ci/rest-compatibility (git timeout)

DaveCTurner

All looks ok to me but I'll let Henning have the final say. I left more suggestions re. the encoding.

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

DaveCTurner · 2021-06-28T14:30:53Z

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

+            final String[] parts = new String(Base64.getDecoder().decode(param), StandardCharsets.UTF_8).split(",");
+            if (parts.length != 3) {
+                throw new IllegalArgumentException(
+                    "after param must be base64 encoded and of the form ${sort_value},${repository_name},${snapshot_name} but was ["


No need to spell out the format here IMO

Suggested change

"after param must be base64 encoded and of the form ${sort_value},${repository_name},${snapshot_name} but was ["

"invalid ?after parameter ["

...est/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponseTests.java

…pshots

original-brownbear · 2021-06-28T16:12:07Z

Thanks @henning and @DaveCTurner! all points addressed now I think :)

original-brownbear · 2021-06-28T16:17:20Z

Sorry @henning wrong Henning. Ment to ping @henningandersen

henningandersen

LGTM.

I think next as a string and after as an object in response/request is slightly inconsistent, but resolving that can be done in a followup rather than in this PR. I think after should turn into a String too, making it opaque at the transport layer too.

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

…pshots

original-brownbear · 2021-06-28T19:55:48Z

Jenkins run elasticsearch-ci/part-1 (unrelated)

original-brownbear · 2021-06-28T20:28:16Z

Thanks Henning + David!

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

Introduce Next Field in Paginated GetSnapshots Response

0092197

Follow up to elastic#73952 adding documentation for the `after` query parameter and the related `next` response field.

original-brownbear added >enhancement :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.14.0 labels Jun 17, 2021

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

original-brownbear added 2 commits June 17, 2021 14:55

cleanup docs

6f41fa6

duration isn't always 0 on ci

622b25b

original-brownbear requested review from DaveCTurner and henningandersen June 17, 2021 15:01

original-brownbear commented Jun 17, 2021

View reviewed changes

original-brownbear mentioned this pull request Jun 21, 2021

Improve Snapshot Repository Scalability #74350

Closed

16 tasks

original-brownbear added 4 commits June 26, 2021 22:35

Merge remote-tracking branch 'elastic/master' into next-field-get-sna…

0f9aad3

…pshots

Merge remote-tracking branch 'elastic/master' into next-field-get-sna…

e178f43

…pshots

fix test

d3b362b

next field correct in all cases

332cbfb

henningandersen reviewed Jun 28, 2021

View reviewed changes

DaveCTurner reviewed Jun 28, 2021

View reviewed changes

original-brownbear added 4 commits June 28, 2021 14:37

Merge remote-tracking branch 'elastic/master' into next-field-get-sna…

60264aa

…pshots

base64 encode param

8a829d1

docs + repo tiebraker

59ac817

Merge remote-tracking branch 'elastic/master' into next-field-get-sna…

70b957f

…pshots

fix param doc stest

27cf07e

DaveCTurner reviewed Jun 28, 2021

View reviewed changes

original-brownbear added 2 commits June 28, 2021 17:06

Merge remote-tracking branch 'elastic/master' into next-field-get-sna…

4da55ff

…pshots

URL-encode base 64

72f29fe

original-brownbear requested a review from henningandersen June 28, 2021 16:05

This comment has been minimized.

Sign in to view

henningandersen approved these changes Jun 28, 2021

View reviewed changes

.../src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java Outdated Show resolved Hide resolved

original-brownbear added 2 commits June 28, 2021 21:19

Merge remote-tracking branch 'elastic/master' into next-field-get-sna…

025116d

…pshots

rename

497333c

original-brownbear merged commit 5f89f8b into elastic:master Jun 28, 2021

original-brownbear deleted the next-field-get-snapshots branch June 28, 2021 20:28

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

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

stevejgordon mentioned this pull request Jul 1, 2021

7.14.0 Meta Ticket elastic/elasticsearch-net#5776

Closed

14 tasks

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

original-brownbear restored the next-field-get-snapshots branch April 18, 2023 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Next Field in Paginated GetSnapshots Response #74236

Introduce Next Field in Paginated GetSnapshots Response #74236

original-brownbear commented Jun 17, 2021

elasticmachine commented Jun 17, 2021

original-brownbear Jun 17, 2021

DaveCTurner Jun 17, 2021

original-brownbear Jun 17, 2021

DaveCTurner Jun 17, 2021

original-brownbear Jun 28, 2021

original-brownbear commented Jun 28, 2021

henningandersen left a comment

henningandersen Jun 28, 2021

original-brownbear Jun 28, 2021

henningandersen Jun 28, 2021

original-brownbear Jun 28, 2021

henningandersen Jun 28, 2021

original-brownbear Jun 28, 2021

henningandersen Jun 28, 2021

original-brownbear Jun 28, 2021

henningandersen Jun 28, 2021

original-brownbear Jun 28, 2021

DaveCTurner left a comment

DaveCTurner Jun 28, 2021

original-brownbear Jun 28, 2021

original-brownbear commented Jun 28, 2021

DaveCTurner left a comment

DaveCTurner Jun 28, 2021

original-brownbear Jun 28, 2021

original-brownbear commented Jun 28, 2021

This comment has been minimized.

original-brownbear commented Jun 28, 2021

henningandersen left a comment

original-brownbear commented Jun 28, 2021

original-brownbear commented Jun 28, 2021

	"after param must be base64 encoded and of the form ${sort_value},${repository_name},${snapshot_name} but was ["
	"invalid ?after parameter ["

Introduce Next Field in Paginated GetSnapshots Response #74236

Introduce Next Field in Paginated GetSnapshots Response #74236

Conversation

original-brownbear commented Jun 17, 2021

elasticmachine commented Jun 17, 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

original-brownbear commented Jun 28, 2021

henningandersen left a 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

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 28, 2021

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 28, 2021

This comment has been minimized.

original-brownbear commented Jun 28, 2021

henningandersen left a comment

Choose a reason for hiding this comment

original-brownbear commented Jun 28, 2021

original-brownbear commented Jun 28, 2021