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

Jump to bottom

Return Total Result Count and Remaining Count in Get Snapshots Response #76150

Merged
merged 3 commits into from
Aug 9, 2021
Merged

Return Total Result Count and Remaining Count in Get Snapshots Response #76150

merged 3 commits into from
Aug 9, 2021

Conversation

original-brownbear
Copy link
Member

@original-brownbear original-brownbear commented Aug 5, 2021 •
edited
Loading

Add total result count and remaining count to get snapshots response.

@original-brownbear
Return Total Result Count and Remaining Count in Get Snapshots Response
3d6d11c 
Add total result count and remaining count to get snapshots response.
@original-brownbear original-brownbear added >enhancement WIP :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Aug 5, 2021
@original-brownbear original-brownbear mentioned this pull request Aug 5, 2021
Closed
16 tasks
original-brownbear
original-brownbear commented Aug 5, 2021
View reviewed changes
...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java Outdated
@@ -156,12 +156,23 @@ private void getMultipleReposSnapshotInfo(
.collect(Collectors.toMap(Tuple::v1, Tuple::v2));
final SnapshotsInRepo snInfos = sortSnapshots(allSnapshots, sortBy, after, size, order);
final List<SnapshotInfo> snapshotInfos = snInfos.snapshotInfos;
final int remaining;
final boolean hasMore = snInfos.hasMore || responses.stream().anyMatch(r -> r.v2() != null && r.v2().hasMore);
Copy link
Member Author

Choose a reason for hiding this comment

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

The logic for all this counting could be made for nicer obviously, but for now I went with the shortest route to getting this implemented on top of the existing functionality as we will be changing how this works under the hood soon anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will remaining not anyway become 0 if hasMore==false? Meaning that we can skip calculating this boolean and just sum up remaining always? And then use that instead of hasMore below? Just seems nicer without too much rewrite.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, dropped hasMore completely now :)

@original-brownbear original-brownbear marked this pull request as ready for review August 5, 2021 11:32
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Aug 5, 2021
@elasticmachine
Copy link
Collaborator

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

henningandersen
henningandersen reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM, left a few minor comments, really all optional.

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java Outdated
@@ -156,12 +156,23 @@ private void getMultipleReposSnapshotInfo(
.collect(Collectors.toMap(Tuple::v1, Tuple::v2));
final SnapshotsInRepo snInfos = sortSnapshots(allSnapshots, sortBy, after, size, order);
final List<SnapshotInfo> snapshotInfos = snInfos.snapshotInfos;
final int remaining;
final boolean hasMore = snInfos.hasMore || responses.stream().anyMatch(r -> r.v2() != null && r.v2().hasMore);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will remaining not anyway become 0 if hasMore==false? Meaning that we can skip calculating this boolean and just sum up remaining always? And then use that instead of hasMore below? Just seems nicer without too much rewrite.

...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java Outdated Show resolved Hide resolved
...src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponse.java Outdated Show resolved Hide resolved
...n/java/org/elasticsearch/action/admin/cluster/snapshots/get/TransportGetSnapshotsAction.java Show resolved Hide resolved
...src/main/java/org/elasticsearch/action/admin/cluster/snapshots/get/GetSnapshotsResponse.java Outdated Show resolved Hide resolved
@original-brownbear
Copy link
Member Author

Thanks Henning!

@original-brownbear original-brownbear merged commit a4983f5 into elastic:master Aug 9, 2021
@original-brownbear original-brownbear deleted the track-total-count branch August 9, 2021 09:34
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Aug 14, 2021
@original-brownbear
Return Total Result Count and Remaining Count in Get Snapshots Respon…
6dbf247 
…se (elastic#76150)

Add total result count and remaining count to get snapshots response.
@original-brownbear original-brownbear mentioned this pull request Aug 14, 2021
Merged
original-brownbear added a commit that referenced this pull request Aug 14, 2021
@original-brownbear
Implement Numeric Pagination in Get Snapshots API (#76532)
d356a4b 
* Return Total Result Count and Remaining Count in Get Snapshots Response (#76150)

Add total result count and remaining count to get snapshots response.

* Implement Numeric Offset Parameter in Get Snapshots API (#76233)

Add numeric offset parameter to this API.

Relates #74350
@Leaf-Lin Leaf-Lin mentioned this pull request Sep 3, 2021
Closed
@original-brownbear original-brownbear restored the track-total-count branch April 18, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henningandersen henningandersen henningandersen approved these changes

@DaveCTurner DaveCTurner Awaiting requested review from DaveCTurner

Assignees
No one assigned
Labels
:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement Team:Distributed Meta label for distributed team v7.15.0 v8.0.0-alpha2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@original-brownbear @elasticmachine @henningandersen @jakelandis @elasticsearchmachine