Pagination and Sorting for Get Snapshots API #73952

original-brownbear · 2021-06-09T13:43:24Z

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.

…shots-api

original-brownbear · 2021-06-09T14:45:41Z

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

@@ -98,6 +98,36 @@ comprising the number of shards in the index, the total size of the index in
 bytes, and the maximum number of segments per shard in the index. Defaults to
 `false`, meaning that this information is omitted.

+`sort`::


The main question left from my end here is on what to do about ?verbose=false requests. For these, the sorting as implemented here will not work out nicely if the request hits and old-version snapshot that doesn't yet track the timestamps in the RepositoryData but will instead just sort by name in this case.

My thinking here would be to maybe just not allow pagination and sorting with ?verbose=false at all for now. Or alternatively, fail these requests if old version snapshots are found in the repo.
Another option would be falling back to loading the missing information needed for pagination when running ?verbose=false queries to work around old repositories but I'm not sure this is worth the effort compared to the other two options.

I think I prefer your first option. Making it dependent on data makes testing difficult.

This would only apply if you specify sort, search or size then?

This would only apply if you specify sort, search or size then?

Yes I think just not allowing any of the three for ?verbose=false would be the plan for option 1.

Went with option 1 in this PR now => no pagination allowed with verbose=false

original-brownbear · 2021-06-09T14:59:10Z

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

+  Sort snapshots by the number of indices they contain and break ties by snapshot name.
+====
+
+`size`::


Also, we talked about this before, we want an upper bound here probably in some implicit + explicit form. I think from a response size perspective I'd go with something like 200 for the upper limit on the size in case it's explicitly set and also return a 400 in case a request that does not have size set would return more than a certain limit number of snapshots. The only issue with this is that currently there are users for whom something like 3k snapshots barely works and having an implicit limit would break their use case potentially.

Could we leave this untouched if none of sort, search, size are specified at least in 7.x?

Could we leave this untouched if none of sort, search, size are specified at least in 7.x?

I wonder if we can/should :) Obviously we don't want to break the API here, but we are moving towards creating situations where users will have a lot more snapshots that before. We did have problems with the memory consumption of the unpaginated API before and although it's better now, I think we are creating situations where a call getting all snapshot verbose could OOM/destabilize a master easily (thinking about shard failures in particular here). If we do keep the unlimited option untouched in 7.x (which makes sense don't get me wrong :)), should we then maybe invest some effort into some sort of circuit breaking during collecting the SnapshotInfo instances to prevent adding a widely used API that can OOM master easily (we could estimate memory from the amount of bytes we read from the blob store or so)?

As I see it, we are not adding such a new API, the existing API has those properties.

I think limiting explicit size in 7.x makes sense and also if you in any way opt-in to the new parts of the API (search, sort, after).

That said, adding some kind of a memory breaker does make sense (but I would prefer to do that after having done all the pagination/scalability improvement work).

Drying up a few spots of code duplication with these tests. Partly to reduce the size of PR elastic#73952 that makes use of the smoke test infrastructure.

original-brownbear · 2021-06-15T09:18:53Z

Thanks Henning + David! I addressed all points now I think and added the possibility to specify sort order as well. This should be good for another round :)

henningandersen

Some more comments, otherwise looking good.

henningandersen · 2021-06-15T11:10:46Z

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

@@ -101,6 +136,20 @@ public ActionRequestValidationException validate() {
        if (repositories == null || repositories.length == 0) {
            validationException = addValidationError("repositories are missing", validationException);
        }
+        if (size == 0) {


Suggested change

if (size == 0) {

if (size == 0 || size < NO_LIMIT) {

henningandersen · 2021-06-15T11:11:42Z

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

+            }
+            if (after != null) {
+                validationException = addValidationError("can't use after with verbose=false", validationException);
+            }


I think we should add a check of order here too.

Also, we should add a minimal test of these validation errors.

++ added that to the validation as well + added a UT for all the validation branches

henningandersen · 2021-06-15T11:29:50Z

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

+Sort order. Valid values are `asc` for ascending and `desc` for descending order. Defaults to `asc`, meaning ascending order.
+
+NOTE: The pagination parameters `size`, `order`, and `sort` are not supported when using `verbose=false` and the sort order for
+requests with `verbose=false` is undefined.


I think we had a well-defined sort order of snapshot name, then with the introduction of start time, it changed to be start-time, then name. Now we are making it "undefined". I see that the implementation still order by start-time and then name by default, so we really did not change anything with this PR. I am inclined to leave this as is, though we could consider calling this a breaking change.

Currently we're only sorting by name for non-verbose (we sort these thin SnapshotInfo that we build in org.elasticsearch.action.admin.cluster.snapshots.get.TransportGetSnapshotsAction#buildSimpleSnapshotInfos). I liked undefined better than starting to explain that when we never actually documented a defined sort order for non-verbose. This change for now won't change anything about that APIs return, just a docs adjustment so not really a breaking change? :)

++, I missed that we still fill in 0L for start time, thanks.

henningandersen · 2021-06-15T12:04:35Z

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

+        int startIndex = 0;
+        if (after != null) {
+            final String name = after.snapshotName();
+            switch (sortBy) {


Can we reuse the comparator to avoid the switching and repetition here? I suppose this is what you meant by your comment above, but unless I am mistaken this would be easy to do?

Not really right? We don't have a SnapshotInfo instance to compare against so we can't use the comparator directly. I refactored the logic here a little now to be drier and use streams to work around the list mutability situation as well, let me know what you think :)

Looks ok, though I think it could be simplified a bit more. But let us tackle that later once we are nearer completion, for the purpose of this PR this is fine.

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

henningandersen · 2021-06-15T12:19:39Z

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

@@ -98,6 +98,37 @@ comprising the number of shards in the index, the total size of the index in
 bytes, and the maximum number of segments per shard in the index. Defaults to
 `false`, meaning that this information is omitted.



The new arguments are indicated as specified in the body, but I think they have to be specified as a query param?

In fact, looking at some of the other params here, it looks as though those also need to be specified as query param, at least ignore_unavailable? I feel like I am missing something that our REST framework handles automatically?

Hmmm, this seems to be a doc bug, sorry for not noticing. The docs are simply wrong here IMO, all of these parameters only work as parts of the URL. We never parse the request body.
I'll adjust the docs accordingly

One second look, we have the same bug in at least the snapshot status API as well where we document all query parameters as body elements but don't actually parse the body. Shall we fix this in a separate PR and audit other docs for the same mistake and leave it as is here maybe?

Ideally, we would just move the new parameters introduced in this PR to the query params section and then fix it in another PR.

But I am also good with your suggestion here if you prefer.

henningandersen · 2021-06-15T12:22:41Z

test/framework/src/main/java/org/elasticsearch/snapshots/AbstractSnapshotIntegTestCase.java

+        return startFullSnapshot(logger, repoName, snapshotName, partial);
+    }
+
+    public static ActionFuture<CreateSnapshotResponse> startFullSnapshot(Logger logger,


Just want to weigh in that I also have a preference (but not a requirement) for static loggers.

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

henningandersen · 2021-06-15T12:43:24Z

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

+import static org.hamcrest.Matchers.in;
+import static org.hamcrest.Matchers.is;
+
+// TODO: dry up duplication across this suite and org.elasticsearch.snapshots.GetSnapshotsIT more


Yeah, that would be nice. Can be done in a follow-up. I would be OK to have a more minimal REST style test and keep the main sort, pagination and stability testing as an internal cluster IT.

I basically just wanted to make extra sure all the param parsing is correct and it seemed easiest to test this by simply running a few cases. My reasoning for this one was that we probably won't save that much code but will lose coverage if we do a simpler rest test that still covers all possible param combinations.

…shots-api

original-brownbear · 2021-06-15T15:40:21Z

Thanks @henningandersen addressed all points I think, this should be ready for another round

henningandersen

LGTM, thanks for the extra iterations Armin.

henningandersen · 2021-06-16T12:24:33Z

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

@@ -98,6 +98,37 @@ comprising the number of shards in the index, the total size of the index in
 bytes, and the maximum number of segments per shard in the index. Defaults to
 `false`, meaning that this information is omitted.



Ideally, we would just move the new parameters introduced in this PR to the query params section and then fix it in another PR.

But I am also good with your suggestion here if you prefer.

henningandersen · 2021-06-16T12:33:18Z

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

+Sort order. Valid values are `asc` for ascending and `desc` for descending order. Defaults to `asc`, meaning ascending order.
+
+NOTE: The pagination parameters `size`, `order`, and `sort` are not supported when using `verbose=false` and the sort order for
+requests with `verbose=false` is undefined.


++, I missed that we still fill in 0L for start time, thanks.

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

henningandersen · 2021-06-16T13:01:50Z

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

+        int startIndex = 0;
+        if (after != null) {
+            final String name = after.snapshotName();
+            switch (sortBy) {


Looks ok, though I think it could be simplified a bit more. But let us tackle that later once we are nearer completion, for the purpose of this PR this is fine.

henningandersen · 2021-06-16T15:05:24Z

...test/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestTests.java

+            final ActionRequestValidationException e = request.validate();
+            assertThat(e.getMessage(), containsString("can't use non-default sort order with verbose=false"));
+        }
+        request.after(new GetSnapshotsRequest.After("foo", "bar"));


nit: We are accumulating errors here, I wonder if we should either do them one by one or just once with all the errors. I prefer one by one, i.e., a new request object for every validation.

++ this was kind of lazy :) I adjusted to testing multiple requests.

…shots-api

original-brownbear · 2021-06-17T07:00:05Z

Thanks Henning + David! Merging here and moving on to implementing next and the search field now.

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

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

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

original-brownbear added 20 commits June 7, 2021 14:23

bck

ee277a3

sorting

d6db405

bck

9d8042b

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

e85c74b

…shots-api

works

b06c720

limit

1bb21ad

bck

6698e00

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

ed601ac

…shots-api

bck

9fd4150

works

5caefe1

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

6b8a221

…shots-api

works

d3d3f60

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

dc5a264

…shots-api

bck

87a8542

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

952fa63

…shots-api

bck

92d1ba0

better test

a602d4f

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

4298bef

…shots-api

works

6372f7e

docs ...

499fb80

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

original-brownbear added 2 commits June 9, 2021 16:07

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

fabdf36

…shots-api

moar coverage

1ed1c37

original-brownbear commented Jun 9, 2021

View reviewed changes

nicer formatting

f8d448d

original-brownbear mentioned this pull request Jun 9, 2021

Dry up HTTP Smoke Tests around Snapshots #73962

Merged

original-brownbear requested review from henningandersen and DaveCTurner June 15, 2021 09:18

henningandersen reviewed Jun 15, 2021

View reviewed changes

original-brownbear added 4 commits June 15, 2021 14:49

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

c1172c8

…shots-api

drier sorting

d903760

validation test

74e1923

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

9a244df

…shots-api

original-brownbear requested a review from henningandersen June 15, 2021 15:39

henningandersen approved these changes Jun 16, 2021

View reviewed changes

original-brownbear added 2 commits June 16, 2021 17:45

Merge remote-tracking branch 'elastic/master' into paginated-get-snap…

53268d5

…shots-api

test nits

93dd6ba

original-brownbear merged commit c1e9590 into elastic:master Jun 17, 2021

original-brownbear deleted the paginated-get-snapshots-api branch June 17, 2021 07:00

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

This was referenced Jun 17, 2021

Introduce Next Field in Paginated GetSnapshots Response #74236

Merged

Improve Snapshot Repository Scalability #74350

Closed

alisonelizabeth mentioned this pull request Jun 24, 2021

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

Merged

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

Introduce Next Field in Paginated GetSnapshots Response (#74236)

5f89f8b

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

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

stevejgordon mentioned this pull request Jul 19, 2022

[FEATURE] Support additional query string parameters for GET Snapshot Repositories elastic/elasticsearch-net#6594

Closed

original-brownbear restored the paginated-get-snapshots-api branch April 18, 2023 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination and Sorting for Get Snapshots API #73952

Pagination and Sorting for Get Snapshots API #73952

original-brownbear commented Jun 9, 2021 •

edited

Loading

original-brownbear Jun 9, 2021

henningandersen Jun 9, 2021

original-brownbear Jun 9, 2021

original-brownbear Jun 10, 2021

original-brownbear Jun 9, 2021

henningandersen Jun 9, 2021

original-brownbear Jun 9, 2021

henningandersen Jun 14, 2021

original-brownbear commented Jun 15, 2021

henningandersen left a comment

henningandersen Jun 15, 2021

henningandersen Jun 15, 2021

original-brownbear Jun 15, 2021

henningandersen Jun 15, 2021

original-brownbear Jun 15, 2021

henningandersen Jun 16, 2021

henningandersen Jun 15, 2021

original-brownbear Jun 15, 2021

henningandersen Jun 16, 2021

henningandersen Jun 15, 2021

original-brownbear Jun 15, 2021

original-brownbear Jun 15, 2021

henningandersen Jun 16, 2021

henningandersen Jun 15, 2021

henningandersen Jun 15, 2021

original-brownbear Jun 15, 2021

original-brownbear commented Jun 15, 2021

henningandersen left a comment

henningandersen Jun 16, 2021

henningandersen Jun 16, 2021

henningandersen Jun 16, 2021

henningandersen Jun 16, 2021

original-brownbear Jun 17, 2021

original-brownbear commented Jun 17, 2021

		@@ -98,6 +98,37 @@ comprising the number of shards in the index, the total size of the index in
		bytes, and the maximum number of segments per shard in the index. Defaults to
		`false`, meaning that this information is omitted.

Pagination and Sorting for Get Snapshots API #73952

Pagination and Sorting for Get Snapshots API #73952

Conversation

original-brownbear commented Jun 9, 2021 • edited Loading

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

original-brownbear commented Jun 15, 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

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

original-brownbear commented Jun 15, 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

original-brownbear commented Jun 17, 2021

original-brownbear commented Jun 9, 2021 •

edited

Loading