Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Numeric Offset Parameter in Get Snapshots API #76233

Merged
merged 3 commits into from
Aug 9, 2021
Merged

Implement Numeric Offset Parameter in Get Snapshots API #76233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d37610
Implement Numeric Offset Parameter in Get Snapshots API
original-brownbear Aug 9, 2021
2690a75
Merge remote-tracking branch 'elastic/master' into allow-get-snapshot…
original-brownbear Aug 9, 2021
284e617
CR: comments
original-brownbear Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions docs/reference/snapshot-restore/apis/get-snapshot-api.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,17 @@ Sort order. Valid values are `asc` for ascending and `desc` for descending order
(Optional, string)
Offset identifier to start pagination from as returned by the `next` field in the response body.

`offset`::
(Optional, integer)
Numeric offset to start pagination from based on the snapshots matching this request. Using a non-zero value for this parameter is mutually
exclusive with using the `after` parameter. Defaults to `0`.

NOTE: The `after` parameter and `next` field allow for iterating through snapshots with some consistency guarantees regarding concurrent
creation or deletion of snapshots. It is guaranteed that any snapshot that exists at the beginning of the iteration and not concurrently
deleted will be seen during the iteration. Snapshots concurrently created may be seen during an iteration.

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

[role="child_attributes"]
[[get-snapshot-api-response-body]]
Expand Down Expand Up @@ -435,6 +440,56 @@ GET /_snapshot/my_repository/snapshot*?size=2&sort=name&after=c25hcHNob3RfMixteV

The API returns the following response:

[source,console-result]
----
{
"snapshots": [
{
"snapshot": "snapshot_3",
"uuid": "dRctdKb54xw67gvLCxSket",
"repository": "my_repository",
"version_id": <version_id>,
"version": <version>,
"indices": [],
"data_streams": [],
"feature_states": [],
"include_global_state": true,
"state": "SUCCESS",
"start_time": "2020-07-06T21:55:18.129Z",
"start_time_in_millis": 1593093628850,
"end_time": "2020-07-06T21:55:18.129Z",
"end_time_in_millis": 1593094752018,
"duration_in_millis": 0,
"failures": [],
"shards": {
"total": 0,
"failed": 0,
"successful": 0
}
}
],
"total": 3,
"remaining": 0
}
----
// TESTRESPONSE[s/"uuid": "dRctdKb54xw67gvLCxSket"/"uuid": $body.snapshots.0.uuid/]
// TESTRESPONSE[s/"version_id": <version_id>/"version_id": $body.snapshots.0.version_id/]
// TESTRESPONSE[s/"version": <version>/"version": $body.snapshots.0.version/]
// TESTRESPONSE[s/"start_time": "2020-07-06T21:55:18.129Z"/"start_time": $body.snapshots.0.start_time/]
// TESTRESPONSE[s/"start_time_in_millis": 1593093628850/"start_time_in_millis": $body.snapshots.0.start_time_in_millis/]
// TESTRESPONSE[s/"end_time": "2020-07-06T21:55:18.129Z"/"end_time": $body.snapshots.0.end_time/]
// TESTRESPONSE[s/"end_time_in_millis": 1593094752018/"end_time_in_millis": $body.snapshots.0.end_time_in_millis/]
// TESTRESPONSE[s/"duration_in_millis": 0/"duration_in_millis": $body.snapshots.0.duration_in_millis/]

Alternatively, the same result could be retrieved by using an offset value of `2` to skip the two snapshot already seen.

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

The API returns the following response:

[source,console-result]
----
{
Expand Down
30 changes: 26 additions & 4 deletions qa/smoke-test-http/src/test/java/org/elasticsearch/http/snapshots/RestGetSnapshotsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,14 @@ private static void assertStablePagination(String repoName,
for (int i = 1; i < allSnapshotNames.size() - j; i++) {
final GetSnapshotsResponse getSnapshotsResponse =
sortedWithLimit(repoName, sort, GetSnapshotsRequest.After.from(after, sort).asQueryParam(), i, order);
final GetSnapshotsResponse getSnapshotsResponseNumeric = sortedWithLimit(repoName, sort, j + 1, i, order);
final List<SnapshotInfo> subsetSorted = getSnapshotsResponse.getSnapshots();
assertEquals(subsetSorted, getSnapshotsResponseNumeric.getSnapshots());
assertEquals(subsetSorted, allSorted.subList(j + 1, j + i + 1));
assertEquals(allSnapshotNames.size(), getSnapshotsResponse.totalCount());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also verify the total count and remaining from the response obtained using offset.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ added assertions for that

assertEquals(allSnapshotNames.size() - (j + i + 1), getSnapshotsResponse.remaining());
assertEquals(getSnapshotsResponseNumeric.totalCount(), getSnapshotsResponse.totalCount());
assertEquals(getSnapshotsResponseNumeric.remaining(), getSnapshotsResponse.remaining());
}
}
}
Expand Down Expand Up @@ -232,10 +236,10 @@ private static GetSnapshotsResponse readSnapshotInfos(Response response) throws
}

private static GetSnapshotsResponse sortedWithLimit(String repoName,
GetSnapshotsRequest.SortBy sortBy,
String after,
int size,
SortOrder order) throws IOException {
GetSnapshotsRequest.SortBy sortBy,
String after,
int size,
SortOrder order) throws IOException {
final Request request = baseGetSnapshotsRequest(repoName);
request.addParameter("sort", sortBy.toString());
if (size != GetSnapshotsRequest.NO_LIMIT || randomBoolean()) {
Expand All @@ -250,4 +254,22 @@ private static GetSnapshotsResponse sortedWithLimit(String repoName,
final Response response = getRestClient().performRequest(request);
return readSnapshotInfos(response);
}

private static GetSnapshotsResponse sortedWithLimit(String repoName,
GetSnapshotsRequest.SortBy sortBy,
int offset,
int size,
SortOrder order) throws IOException {
final Request request = baseGetSnapshotsRequest(repoName);
request.addParameter("sort", sortBy.toString());
if (size != GetSnapshotsRequest.NO_LIMIT || randomBoolean()) {
request.addParameter("size", String.valueOf(size));
}
request.addParameter("offset", String.valueOf(offset));
if (order == SortOrder.DESC || randomBoolean()) {
request.addParameter("order", order.toString());
}
final Response response = getRestClient().performRequest(request);
return readSnapshotInfos(response);
}
}
21 changes: 15 additions & 6 deletions server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,11 +197,15 @@ private static void assertStablePagination(String repoName, Collection<String> a
i,
order
);
final GetSnapshotsResponse getSnapshotsResponseNumeric = sortedWithLimit(repoName, sort, j + 1, i, order);
final List<SnapshotInfo> subsetSorted = getSnapshotsResponse.getSnapshots();
assertEquals(subsetSorted, getSnapshotsResponseNumeric.getSnapshots());
assertEquals(subsetSorted, allSorted.subList(j + 1, j + i + 1));
assertEquals(allSnapshotNames.size(), getSnapshotsResponse.totalCount());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also verify the total count and remaining from the response obtained using offset.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ added assertions for that

assertEquals(allSnapshotNames.size() - (j + i + 1), getSnapshotsResponse.remaining());
assertEquals(subsetSorted, allSorted.subList(j + 1, j + i + 1));
assertEquals(getSnapshotsResponseNumeric.totalCount(), getSnapshotsResponse.totalCount());
assertEquals(getSnapshotsResponseNumeric.remaining(), getSnapshotsResponse.remaining());
}
}
}
Expand Down Expand Up @@ -230,12 +234,17 @@ private static GetSnapshotsResponse sortedWithLimit(
int size,
SortOrder order
) {
final GetSnapshotsResponse response = baseGetSnapshotsRequest(repoName).setAfter(after)
.setSort(sortBy)
.setSize(size)
.setOrder(order)
.get();
return response;
return baseGetSnapshotsRequest(repoName).setAfter(after).setSort(sortBy).setSize(size).setOrder(order).get();
}

private static GetSnapshotsResponse sortedWithLimit(
String repoName,
GetSnapshotsRequest.SortBy sortBy,
int offset,
int size,
SortOrder order
) {
return baseGetSnapshotsRequest(repoName).setOffset(offset).setSort(sortBy).setSize(size).setOrder(order).get();
}

private static GetSnapshotsRequestBuilder baseGetSnapshotsRequest(String repoName) {
Expand Down
29 changes: 29 additions & 0 deletions ...c/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ public class GetSnapshotsRequest extends MasterNodeRequest<GetSnapshotsRequest>
*/
private int size = NO_LIMIT;

/**
* Numeric offset at which to start fetching snapshots. Mutually exclusive with {@link After} if not equal to {@code 0}.
*/
private int offset = 0;

@Nullable
private After after;

Expand Down Expand Up @@ -104,6 +109,9 @@ public GetSnapshotsRequest(StreamInput in) throws IOException {
sort = in.readEnum(SortBy.class);
size = in.readVInt();
order = SortOrder.readFromStream(in);
if (in.getVersion().onOrAfter(NUMERIC_PAGINATION_VERSION)) {
offset = in.readVInt();
}
}
}

Expand All @@ -130,6 +138,13 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeEnum(sort);
out.writeVInt(size);
order.writeTo(out);
if (out.getVersion().onOrAfter(NUMERIC_PAGINATION_VERSION)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we not want to fail if offset is non-zero when talking to a too old version? Just like 3 lines down.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ added handling for that thanks!

out.writeVInt(offset);
} else if (offset != 0) {
throw new IllegalArgumentException(
"can't use numeric offset in get snapshots request with node version [" + out.getVersion() + "]"
);
}
} else if (sort != SortBy.START_TIME || size != NO_LIMIT || after != null || order != SortOrder.ASC) {
throw new IllegalArgumentException("can't use paginated get snapshots request with node version [" + out.getVersion() + "]");
}
Expand All @@ -151,12 +166,17 @@ public ActionRequestValidationException validate() {
if (size > 0) {
validationException = addValidationError("can't use size limit with verbose=false", validationException);
}
if (offset > 0) {
validationException = addValidationError("can't use offset with verbose=false", validationException);
}
if (after != null) {
validationException = addValidationError("can't use after with verbose=false", validationException);
}
if (order != SortOrder.ASC) {
validationException = addValidationError("can't use non-default sort order with verbose=false", validationException);
}
} else if (after != null && offset > 0) {
validationException = addValidationError("can't use after and offset simultaneously", validationException);
}
return validationException;
}
Expand Down Expand Up @@ -265,6 +285,15 @@ public int size() {
return size;
}

public int offset() {
return offset;
}

public GetSnapshotsRequest offset(int offset) {
this.offset = offset;
return this;
}

public SortOrder order() {
return order;
}
Expand Down
5 changes: 5 additions & 0 deletions ...java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ public GetSnapshotsRequestBuilder setSize(int size) {
return this;
}

public GetSnapshotsRequestBuilder setOffset(int offset) {
request.offset(offset);
return this;
}

public GetSnapshotsRequestBuilder setOrder(SortOrder order) {
request.order(order);
return this;
Expand Down
24 changes: 11 additions & 13 deletions ...ava/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ protected void masterOperation(
(CancellableTask) task,
request.sort(),
request.after(),
request.offset(),
request.size(),
request.order(),
listener
Expand All @@ -133,6 +134,7 @@ private void getMultipleReposSnapshotInfo(
CancellableTask cancellableTask,
GetSnapshotsRequest.SortBy sortBy,
@Nullable GetSnapshotsRequest.After after,
int offset,
int size,
SortOrder order,
ActionListener<GetSnapshotsResponse> listener
Expand All @@ -154,7 +156,7 @@ private void getMultipleReposSnapshotInfo(
.map(Tuple::v1)
.filter(Objects::nonNull)
.collect(Collectors.toMap(Tuple::v1, Tuple::v2));
final SnapshotsInRepo snInfos = sortSnapshots(allSnapshots, sortBy, after, size, order);
final SnapshotsInRepo snInfos = sortSnapshots(allSnapshots, sortBy, after, offset, size, order);
final List<SnapshotInfo> snapshotInfos = snInfos.snapshotInfos;
final int remaining = snInfos.remaining + responses.stream()
.map(Tuple::v2)
Expand Down Expand Up @@ -183,7 +185,6 @@ private void getMultipleReposSnapshotInfo(
cancellableTask,
sortBy,
after,
size,
order,
groupedActionListener.delegateResponse((groupedListener, e) -> {
if (isMultiRepoRequest && e instanceof ElasticsearchException) {
Expand All @@ -205,7 +206,6 @@ private void getSingleRepoSnapshotInfo(
CancellableTask task,
GetSnapshotsRequest.SortBy sortBy,
@Nullable final GetSnapshotsRequest.After after,
int size,
SortOrder order,
ActionListener<SnapshotsInRepo> listener
) {
Expand Down Expand Up @@ -237,7 +237,6 @@ private void getSingleRepoSnapshotInfo(
task,
sortBy,
after,
size,
order,
listener
),
Expand Down Expand Up @@ -277,7 +276,6 @@ private void loadSnapshotInfos(
CancellableTask task,
GetSnapshotsRequest.SortBy sortBy,
@Nullable final GetSnapshotsRequest.After after,
int size,
SortOrder order,
ActionListener<SnapshotsInRepo> listener
) {
Expand Down Expand Up @@ -327,22 +325,22 @@ private void loadSnapshotInfos(
task,
sortBy,
after,
size,
order,
listener
);
} else {
final SnapshotsInRepo snapshotInfos;
if (repositoryData != null) {
// want non-current snapshots as well, which are found in the repository data
snapshotInfos = buildSimpleSnapshotInfos(toResolve, repo, repositoryData, currentSnapshots, sortBy, after, size, order);
snapshotInfos = buildSimpleSnapshotInfos(toResolve, repo, repositoryData, currentSnapshots, sortBy, after, order);
} else {
// only want current snapshots
snapshotInfos = sortSnapshots(
currentSnapshots.stream().map(SnapshotInfo::basic).collect(Collectors.toList()),
sortBy,
after,
size,
0,
GetSnapshotsRequest.NO_LIMIT,
order
);
}
Expand All @@ -365,7 +363,6 @@ private void snapshots(
CancellableTask task,
GetSnapshotsRequest.SortBy sortBy,
@Nullable GetSnapshotsRequest.After after,
int size,
SortOrder order,
ActionListener<SnapshotsInRepo> listener
) {
Expand Down Expand Up @@ -395,7 +392,7 @@ private void snapshots(
final ActionListener<Void> allDoneListener = listener.delegateFailure((l, v) -> {
final ArrayList<SnapshotInfo> snapshotList = new ArrayList<>(snapshotInfos);
snapshotList.addAll(snapshotSet);
listener.onResponse(sortSnapshots(snapshotList, sortBy, after, size, order));
listener.onResponse(sortSnapshots(snapshotList, sortBy, after, 0, GetSnapshotsRequest.NO_LIMIT, order));
});
if (snapshotIdsToIterate.isEmpty()) {
allDoneListener.onResponse(null);
Expand Down Expand Up @@ -445,7 +442,6 @@ private static SnapshotsInRepo buildSimpleSnapshotInfos(
final List<SnapshotInfo> currentSnapshots,
final GetSnapshotsRequest.SortBy sortBy,
@Nullable final GetSnapshotsRequest.After after,
final int size,
final SortOrder order
) {
List<SnapshotInfo> snapshotInfos = new ArrayList<>();
Expand Down Expand Up @@ -475,7 +471,7 @@ private static SnapshotsInRepo buildSimpleSnapshotInfos(
)
);
}
return sortSnapshots(snapshotInfos, sortBy, after, size, order);
return sortSnapshots(snapshotInfos, sortBy, after, 0, GetSnapshotsRequest.NO_LIMIT, order);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I just dropped the size and offset limits everywhere but before resolving the final listener.
There isn't really an easy way of implementing a numeric offset with the current approach otherwise and the memory savings from dropping a few elements from the stream earlier are probably trivial anyway. For now this seems good enough to me and once this is in and the search parameter PR has been merged we can start optimizing the code here I think.

}

private static final Comparator<SnapshotInfo> BY_START_TIME = Comparator.comparingLong(SnapshotInfo::startTime)
Expand All @@ -494,6 +490,7 @@ private static SnapshotsInRepo sortSnapshots(
final List<SnapshotInfo> snapshotInfos,
final GetSnapshotsRequest.SortBy sortBy,
final @Nullable GetSnapshotsRequest.After after,
final int offset,
final int size,
final SortOrder order
) {
Expand All @@ -518,6 +515,7 @@ private static SnapshotsInRepo sortSnapshots(
Stream<SnapshotInfo> infos = snapshotInfos.stream();

if (after != null) {
assert offset == 0 : "can't combine after and offset but saw [" + after + "] and offset [" + offset + "]";
final Predicate<SnapshotInfo> isAfter;
final String snapshotName = after.snapshotName();
final String repoName = after.repoName();
Expand Down Expand Up @@ -553,7 +551,7 @@ private static SnapshotsInRepo sortSnapshots(
}
infos = infos.filter(isAfter);
}
infos = infos.sorted(order == SortOrder.DESC ? comparator.reversed() : comparator);
infos = infos.sorted(order == SortOrder.DESC ? comparator.reversed() : comparator).skip(offset);
final List<SnapshotInfo> allSnapshots = infos.collect(Collectors.toUnmodifiableList());
final List<SnapshotInfo> snapshots;
if (size != GetSnapshotsRequest.NO_LIMIT) {
Expand Down
Loading