Implement from_sort_value Parameter in Get Snapshots API by original-brownbear · Pull Request #77618 · elastic/elasticsearch

original-brownbear · 2021-09-13T12:14:31Z

Add from_sort_value parameter to allow for filtering snapshots by comparing to concrete sort column
values similar to the existing after parameter`.

Add `after_value` parameter to allow for filtering snapshots by comparing to concrete sort column values similar to the existing `after` parameter`.

elasticmachine · 2021-09-13T12:14:34Z

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

original-brownbear · 2021-09-13T12:15:57Z

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

        assertThat(getAllSnapshotsForPolicies(GetSnapshotsRequest.NO_POLICY_PATTERN, "*"), is(allSnapshots));
    }

+    public void testSortAfterStartTime() {


I did not explicitly add tests for all columns as they use the same functionality and limited myself to those that I needed for full coverage only here.

original-brownbear · 2021-09-13T12:19:25Z

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

        SortOrder order
    ) {
+        if (snapshotName == null) {
+            assert repoName == null : "no snapshot name given but saw repo name [" + repoName + "]";


There's definitely ways of making this nicer, but since it's a temporary solution I went for the shortest+easiest-to-review version again to get this into the API (and thus complete the API side of this work) asap.

original-brownbear · 2021-09-14T09:01:21Z

Jenkins run elasticsearch-ci/part-1

original-brownbear · 2021-09-14T09:36:27Z

@henningandersen this one should be good for review now :) as discussed it's using >= all the way now.

tlrx · 2021-09-14T09:51:34Z

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

+        blockAllDataNodes(repoName);
+        final ActionFuture<CreateSnapshotResponse> snapshot2Future = startFullSnapshot(repoName, "snapshot-2");
+        awaitNumberOfSnapshotsInProgress(1);
+        TimeUnit.MILLISECONDS.sleep(snapshot1.endTime() - snapshot1.startTime() + 1);


I'm not sure if this complexity really worth it? Maybe we could just snapshot indices with random docs, retrieve the duration and test after_value after that?

Agreed, this seems overly complex. I think we could even just do one test with all 3 validations in one go?

It's pretty easy to get collisions no matter what things you do in between snapshots if you don't make it deterministic like this. That said, this wasn't even good enough for Windows I think and I went with a different approach now, running the snapshot create in a loop until we get unique timestamps. Should be safe on all systems now :)

tlrx · 2021-09-14T10:05:07Z

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

 // TESTRESPONSE[s/"duration_in_millis": 1/"duration_in_millis": $body.snapshots.1.duration_in_millis/]
+
+
+The following request returns information for all snapshots that come after `snapshot_2` when sorted by snapshot name in the default


I suspect that sorting after a given start/end time will be the most common use case. Maybe it's worth showing as an example?

I'd love to but I don't see a way of doing this with a test because the timestamp on the request would have to be dynamic right?

the timestamp on the request would have to be dynamic right?

Or low enough to always return some snapshots in the response. But that's just a suggestion, let's ignore if that's too complicated or ugly.

Fair point :) Added a docs run for this with a low timestamp now.

henningandersen

Left a few smaller comments, otherwise this looks good.

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

henningandersen · 2021-09-14T11:02:04Z

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

 (Optional, string)
 Sort order. Valid values are `asc` for ascending and `desc` for descending order. Defaults to `asc`, meaning ascending order.

+`after_value`::


I wonder if we should call the parameter after_sort_value to make the coupling to sort explicit in the name?

I am still pondering on after_. Reading your text, perhaps start_sort_value is better, though I am not really contend with that either...

Not sure about this or the alternatives. How about just from maybe? It's short and it indicates "start" + "inclusive"?

I like from, but perhaps still qualify it as from_sort_value? I think that makes it easier to see from the request what it means.

henningandersen · 2021-09-14T11:04:10Z

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

+
+        assertThat(allAfterStartTimeAscending(startTime1 - 1), is(allSnapshotInfo));
+        assertThat(allAfterStartTimeAscending(startTime1), is(allSnapshotInfo));
+        assertThat(allAfterStartTimeAscending(startTime2), is(List.of(snapshot2, snapshot3)));


Are we sure that snapshot1 and snapshot2 get different timestamps?

Maybe not on Windows where the clock is less accurate actually ... let me make sure they will.

Adjusted the code to ensure the timestamps never collide now.

I am not sure I see that here in the rest test case, only in the internal cluster test? Maybe I missed it?

Oh right missed this one, adding a fix here as well :)

henningandersen · 2021-09-14T11:07:57Z

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

+            .getSnapshots();
+    }
+
+    public void testSortAfterName() {


This test and the one sorting by timestamp are very similar, perhaps we can share most of the code?

Merged all tests into one :)

henningandersen · 2021-09-14T11:10:52Z

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

+        blockAllDataNodes(repoName);
+        final ActionFuture<CreateSnapshotResponse> snapshot2Future = startFullSnapshot(repoName, "snapshot-2");
+        awaitNumberOfSnapshotsInProgress(1);
+        TimeUnit.MILLISECONDS.sleep(snapshot1.endTime() - snapshot1.startTime() + 1);


Agreed, this seems overly complex. I think we could even just do one test with all 3 validations in one go?

henningandersen · 2021-09-14T11:13:55Z

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

+            if (after != null) {
+                validationException = addValidationError("can't use after and offset simultaneously", validationException);
+            }
+            if (afterValue != null) {


Is there a reason we are not allowing this? offset could just be evaluated after after_value?

Obviously we can do that in a follow-up rather than here.

Huh yea actually we have to that for Kibana (otherwise any filtered results can't be paginated through which seems weird). Let's push it to a follow-up though as it will require some more code-heavy tests I think, though the production code change is easy/small :)

Nevermind, this turned out to be entirely trivial with the way offset works at the moment. I just enabled it and added 2 simple tests for offset + after.

henningandersen · 2021-09-14T11:15:01Z

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


-NOTE: The parameters `size`, `order`, `after`, `offset`, `slm_policy_filter` and `sort` are not supported when using `verbose=false` and
-the sort order for requests with `verbose=false` is undefined.
+NOTE: The parameters `size`, `order`, `after`, `after_value`, `offset`, `slm_policy_filter` and `sort` are not supported when using


We should also note the incompatibility of after_value with offset and after?

Well now that you pointed it out below, it would only be with "after" in the final thing which I'd code up very shortly after this one. Not sure it's worth the added text though, it obviously makes no sense setting both, the request validation will tell you so and we already point this out in the after section?

I see you added the incompatibility note to offset, I missed that on my initial read. I think it still makes sense to add to either after or after_value that they are incompatible. I could see this being slightly surprising, since after_value is like a filter so it could be surprising that it does not work with pagination using after (and this could slip through some UI testing).

Done, added a note to after :)

henningandersen · 2021-09-14T11:24:38Z

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

+        final Predicate<SnapshotInfo> isAfter;
+        switch (sortBy) {
+            case START_TIME:
+                isAfter = filterByLongOffset(SnapshotInfo::startTime, Long.parseLong(after), snapshotName, repoName, order);


I think we should add explicit error handling when parsing the arguments, perhaps to the request validation or here. That would allow the message to indicate that it is the after_value causing the problem.

Also, I think < 0 is illegal so we could guard against that.

Similarly for duration, indices and failed shards.

I just realized we have the safe problem for the after value as well (though due to the way we encode things there it's less likely). May I push this into a follow-up? :) Then I can just deal with both in one go and make it a little nicer without blowing this one up.

That said, I refactored this a little to build the predicate early on now. This makes sure that we at least don't run any actual requests unless we have a valid predicate. Refactoring this nicer and working in a fix for the after param + easy-to-read validation I'd still like to push to a follow-up though if possible :)

I think that for after we expect it to be handle opaquely by clients so if they mess with it, I am OK to have a worse error (though I will not object to improving it).

I am OK with pushing the error handling to a separate PR.

henningandersen · 2021-09-14T11:34:17Z

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

+        assertThat(allBeforeStartTimeDescending(startTime2), is(List.of(snapshot2, snapshot1)));
+        assertThat(allBeforeStartTimeDescending(startTime1), is(List.of(snapshot1)));
+        assertThat(allBeforeStartTimeDescending(startTime1 - 1), empty());
+    }


Can we add a combination test too, validating that after_value and for instance snapshot name filtering works together? Just one of these is enough, no need to do several combinations.

Added a test for that and also for offset + after_value which turned out to be trivial.

original-brownbear · 2021-09-14T14:01:32Z

Thanks so much for reading through this monster @henningandersen @tlrx . I think I was able to work in all your points now :) The BwC failure is unrelated and know and this should be good for another review I hope.

henningandersen

Thanks Armin, this looks good, just a comment on the name and some minor comments.

henningandersen · 2021-09-15T09:41:45Z

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

 (Optional, string)
 Sort order. Valid values are `asc` for ascending and `desc` for descending order. Defaults to `asc`, meaning ascending order.

+`after_value`::


I like from, but perhaps still qualify it as from_sort_value? I think that makes it easier to see from the request what it means.

henningandersen · 2021-09-15T09:42:45Z

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

 (Optional, string)
-Offset identifier to start pagination from as returned by the `next` field in the response body.
+Offset identifier to start pagination from as returned by the `next` field in the response body. Using this parameter is mutually exclusive
+with using the `after_value` parameter.


I think I saw a comment where you mentioned you did this anyway since it was so easy?

I didn't do it for after, just for numeric offsets. We don't want after and from_sort_value working in parallel do we?

Sorry, I got confused by the "offset" part of the sentence, this is fine as is. About after and from_sort_value, let us see what kibana thinks, I think they can handle it.

henningandersen · 2021-09-15T09:48:37Z

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

+
+        assertThat(allAfterStartTimeAscending(startTime1 - 1), is(allSnapshotInfo));
+        assertThat(allAfterStartTimeAscending(startTime1), is(allSnapshotInfo));
+        assertThat(allAfterStartTimeAscending(startTime2), is(List.of(snapshot2, snapshot3)));


I am not sure I see that here in the rest test case, only in the internal cluster test? Maybe I missed it?

henningandersen · 2021-09-15T09:59:49Z

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

+    ) throws Exception {
+        while (true) {
+            final SnapshotInfo snapshotInfo = createFullSnapshot(repoName, snapshotName);
+            final long duration = snapshotInfo.endTime() - snapshotInfo.startTime();


I am mildly concerned about the duration being either 0 or 100ms on some platform meaning we could loop infinitely here. I think we will be ok so let us leave it, though a comment to not go beyond the 3 snapshots we generate now seems in order.

Good point, comment added

original-brownbear · 2021-09-15T10:56:55Z

Thanks @henningandersen renamed + other comments addressed as well now :)

henningandersen

LGTM.

tlrx

LGTM

tlrx · 2021-09-15T10:13:43Z

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

 // TESTRESPONSE[s/"duration_in_millis": 1/"duration_in_millis": $body.snapshots.1.duration_in_millis/]
+
+
+The following request returns information for all snapshots that come after `snapshot_2` when sorted by snapshot name in the default


original-brownbear · 2021-09-15T11:36:45Z

Thanks Henning & Tanguy!!

…9318) Add `from_sort_value` parameter to allow for filtering snapshots by comparing to concrete sort column values similar to the existing after parameter`.

Implement after_value Parameter in Get Snapshots API

e2529d5

Add `after_value` parameter to allow for filtering snapshots by comparing to concrete sort column values similar to the existing `after` parameter`.

original-brownbear added >enhancement :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.16.0 labels Sep 13, 2021

elasticmachine added the Team:Distributed Meta label for distributed team. label Sep 13, 2021

original-brownbear mentioned this pull request Sep 13, 2021

Improve Snapshot Repository Scalability #74350

Closed

16 tasks

original-brownbear commented Sep 13, 2021

View reviewed changes

noise reduction

130344a

original-brownbear commented Sep 13, 2021

View reviewed changes

original-brownbear requested review from henningandersen and tlrx September 13, 2021 13:12

original-brownbear added 2 commits September 14, 2021 10:25

Merge remote-tracking branch 'elastic/master' into after-value

baec0ea

lte gte

6f0ae18

tlrx reviewed Sep 14, 2021

View reviewed changes

henningandersen reviewed Sep 14, 2021

View reviewed changes

original-brownbear added 5 commits September 14, 2021 13:58

Merge remote-tracking branch 'elastic/master' into after-value

0fa188f

enable offset, CR comments on tests

2ece405

Merge remote-tracking branch 'elastic/master' into after-value

7b30c21

Merge remote-tracking branch 'elastic/master' into after-value

def381c

redo predicate a little

e4fd7fc

original-brownbear requested review from henningandersen and tlrx September 14, 2021 14:00

original-brownbear added 2 commits September 14, 2021 17:15

Merge remote-tracking branch 'elastic/master' into after-value

089b4b4

typo

960a2c1

henningandersen reviewed Sep 15, 2021

View reviewed changes

original-brownbear added 4 commits September 15, 2021 12:16

Merge remote-tracking branch 'elastic/master' into after-value

e34c61c

CR: comments

59fb31d

Merge remote-tracking branch 'elastic/master' into after-value

f7ec41a

cosmetics

108367a

original-brownbear requested a review from henningandersen September 15, 2021 10:56

henningandersen approved these changes Sep 15, 2021

View reviewed changes

tlrx approved these changes Sep 15, 2021

View reviewed changes

original-brownbear changed the title ~~Implement after_value Parameter in Get Snapshots API~~ Implement from_sort_value Parameter in Get Snapshots API Sep 15, 2021

original-brownbear merged commit 2544d91 into elastic:master Sep 15, 2021

original-brownbear deleted the after-value branch September 15, 2021 11:37

original-brownbear added the backport pending label Sep 15, 2021

original-brownbear mentioned this pull request Oct 17, 2021

Implement from_sort_value Parameter in Get Snapshots API (#77618) #79318

Merged

original-brownbear removed the backport pending label Oct 17, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

original-brownbear restored the after-value branch April 18, 2023 21:03

		// TESTRESPONSE[s/"duration_in_millis": 1/"duration_in_millis": $body.snapshots.1.duration_in_millis/]


		The following request returns information for all snapshots that come after `snapshot_2` when sorted by snapshot name in the default

Conversation

original-brownbear commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 14, 2021

Uh oh!

original-brownbear commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 13, 2021 •

edited

Loading