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

Support v7 response format for get-snapshots API #69108

Closed
DaveCTurner opened this issue Feb 17, 2021 · 4 comments · Fixed by #74451
Closed

Support v7 response format for get-snapshots API #69108

DaveCTurner opened this issue Feb 17, 2021 · 4 comments · Fixed by #74451
Assignees
@original-brownbear
Labels
blocker >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.0.0-alpha1

Comments

@DaveCTurner
Copy link
Contributor

In #42090 we changed the response format for the get-snapshots action in 8.0. When the work to support versioned response formats lands, we should revisit this breaking change and make sure we continue to return the older format if requested.
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Feb 17, 2021
@elasticmachine
Copy link
Collaborator

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

@pgomulka pgomulka mentioned this issue Mar 11, 2021
Closed
@pgomulka pgomulka self-assigned this Jun 23, 2021
@pgomulka
Copy link
Contributor

so for v7 response:
if someone is using the regular /_snapshot/{repository}/{snapshot} with repository being exact name (not all, or * or multiple values) I could just iterate over successfulResponses and generate a response

if the failedResponse was not empty, then I would return the exception - I would not even iterate the successfulResponses.
Should I just take first failure from failedResponse? previously in 7 we would not expect a list of exceptions, but only one.

I don't expect for singleRepo, singleSnapshot api to return mix of successful and failed response in v7. Is this assumption ok?

now if someone would try to use /_snapshot/all/all with v7 api, then it would get tricky. I have a feeling that this is unlikely as folks would not have their apps/clients updated to do this.
For simplicity I thought I could just iterate all successfulResponses and merge them all together (skip the repository json level)
However failedResponse was not empty, then for the reason mentioned above, I would not bother iterating successfulResponses.

What do you think?

@original-brownbear
Copy link
Member

@pgomulka sorry for not spotting this earlier! I'm actually working on exactly this issue in #74451 right now which will make v8 mostly use the v7 format again for requests that v7 APIs can respond to. Would you be ok with me assigning this to myself?

@pgomulka
Copy link
Contributor

pgomulka commented Jun 23, 2021 •
edited
Loading

We discussed this with Armin and there is a chance that there possibly won't be a breaking change for the get snapshots.
You can ignore my comments

@original-brownbear original-brownbear mentioned this issue Jun 23, 2021
Merged
original-brownbear added a commit that referenced this issue Jun 24, 2021
@original-brownbear
Flatten Get Snapshots Response (#74451)
cbf48e0 
This PR returns the get snapshots API to the 7.x format (and transport client behavior) and enhances it for requests that ask for multiple repositories.
The changes for requests that target multiple repositories are:
* Add `repository` field to `SnapshotInfo` and REST response
* Add `failures` map alongside `snapshots` list instead of returning just an exception response as done for single repo requests
* Pagination now works across repositories instead of being per repository for multi-repository requests

closes #69108
closes #43462
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this issue Jun 29, 2021
@original-brownbear
Flatten Get Snapshots Response (elastic#74451)
307dbc6 
This PR returns the get snapshots API to the 7.x format (and transport client behavior) and enhances it for requests that ask for multiple repositories.
The changes for requests that target multiple repositories are:
* Add `repository` field to `SnapshotInfo` and REST response
* Add `failures` map alongside `snapshots` list instead of returning just an exception response as done for single repo requests
* Pagination now works across repositories instead of being per repository for multi-repository requests

closes elastic#69108
closes elastic#43462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@original-brownbear original-brownbear

Labels
blocker >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Flatten Get Snapshots Response original-brownbear/elasticsearch
5 participants
@jakelandis @DaveCTurner @original-brownbear @pgomulka @elasticmachine