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.

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

Output all empty snapshot info fields if in verbose mode #25455

Merged
merged 2 commits into from Jun 28, 2017

Conversation

Projects
None yet
5 participants
@abeyad
Contributor

abeyad commented Jun 28, 2017

In #24477, a less verbose option was added to retrieve snapshot info via
GET /_snapshot/{repo}/{snapshots}. The point of adding this less
verbose option was so that if the repository is a cloud based one, and
there are many snapshots for which the snapshot info needed to be
retrieved, then each snapshot would require reading a separate snapshot
metadata file to pull out the necessary information. This can be costly
(performance and cost) on cloud based repositories, so a less verbose
option was added that only retrieves very basic information about each
snapshot that is all available in the index-N blob - requiring only one
read!

In order to display this less verbose snapshot info appropriately, logic
was added to not display those fields which could not be populated.
However, this broke integrators (e.g. ECE) that required these fields to
be present, even if empty. This commit is to return these fields in the
response, even if empty, if the verbose option is set.

Ali Beyad
Output all empty snapshot info fields if in verbose mode
In #24477, a less verbose option was added to retrieve snapshot info via
GET /_snapshot/{repo}/{snapshots}.  The point of adding this less
verbose option was so that if the repository is a cloud based one, and
there are many snapshots for which the snapshot info needed to be
retrieved, then each snapshot would require reading a separate snapshot
metadata file to pull out the necessary information.  This can be costly
(performance and cost) on cloud based repositories, so a less verbose
option was added that only retrieves very basic information about each
snapshot that is all available in the index-N blob - requiring only one
read!

In order to display this less verbose snapshot info appropriately, logic
was added to not display those fields which could not be populated.
However, this broke integrators (e.g. ECE) that required these fields to
be present, even if empty.  This commit is to return these fields in the
response, even if empty, if the verbose option is set.

@abeyad abeyad changed the title from Output all SnapshotInfo fields in returned x-content to Output all empty snapshot info fields if in verbose mode Jun 28, 2017

@imotov

imotov approved these changes Jun 28, 2017

Left one comment regarding dealing with params. I think it can be simplified. Otherwise, LGTM.

new RestBuilderListener<GetSnapshotsResponse>(channel) {
@Override
public RestResponse buildResponse(GetSnapshotsResponse response, XContentBuilder builder) throws Exception {
Map<String, String> params = Collections.singletonMap("verbose", Boolean.toString(getSnapshotsRequest.verbose()));

This comment has been minimized.

@imotov

imotov Jun 28, 2017

Member

I don't think is necessary. request is passed asparams. So, "verbose" is already in params if it was specified, you just need to be careful resolving defaults. You would also miss other parameters here such as human and pretty.

Ali Beyad
@abeyad

This comment has been minimized.

Contributor

abeyad commented Jun 28, 2017

I pushed 3d34d08 to address your comment. Thanks for the review @imotov!

@rjernst

This comment has been minimized.

Member

rjernst commented Jun 28, 2017

I understand the reason for this remaining the "default" behavior in 5.x, so as not to break compatibility within a major, but we should still be able to remove this long term? As a followup can the shorter form be used when a url param is set, and that then become the default in 6.0?

@abeyad

This comment has been minimized.

Contributor

abeyad commented Jun 28, 2017

@rjernst I had a discussion with @clintongormley about this a few weeks ago and thought maybe we should make the default verbose=false for 6.x, but didn't reach a decision at the time. @clintongormley what do you think?

@abeyad abeyad merged commit b18bfd6 into elastic:master Jun 28, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@abeyad abeyad deleted the abeyad:complete_snapshot_info branch Jun 28, 2017

abeyad pushed a commit that referenced this pull request Jun 28, 2017

Ali Beyad
Output all empty snapshot info fields if in verbose mode (#25455)
In #24477, a less verbose option was added to retrieve snapshot info via
GET /_snapshot/{repo}/{snapshots}.  The point of adding this less
verbose option was so that if the repository is a cloud based one, and
there are many snapshots for which the snapshot info needed to be
retrieved, then each snapshot would require reading a separate snapshot
metadata file to pull out the necessary information.  This can be costly
(performance and cost) on cloud based repositories, so a less verbose
option was added that only retrieves very basic information about each
snapshot that is all available in the index-N blob - requiring only one
read!

In order to display this less verbose snapshot info appropriately, logic
was added to not display those fields which could not be populated.
However, this broke integrators (e.g. ECE) that required these fields to
be present, even if empty.  This commit is to return these fields in the
response, even if empty, if the verbose option is set.

abeyad pushed a commit that referenced this pull request Jun 28, 2017

Ali Beyad
Output all empty snapshot info fields if in verbose mode (#25455)
In #24477, a less verbose option was added to retrieve snapshot info via
GET /_snapshot/{repo}/{snapshots}.  The point of adding this less
verbose option was so that if the repository is a cloud based one, and
there are many snapshots for which the snapshot info needed to be
retrieved, then each snapshot would require reading a separate snapshot
metadata file to pull out the necessary information.  This can be costly
(performance and cost) on cloud based repositories, so a less verbose
option was added that only retrieves very basic information about each
snapshot that is all available in the index-N blob - requiring only one
read!

In order to display this less verbose snapshot info appropriately, logic
was added to not display those fields which could not be populated.
However, this broke integrators (e.g. ECE) that required these fields to
be present, even if empty.  This commit is to return these fields in the
response, even if empty, if the verbose option is set.
@abeyad

This comment has been minimized.

Contributor

abeyad commented Jun 28, 2017

5.x commit: 9501233
5.5 commit: 3afbafa

@clintongormley clintongormley added the >bug label Jun 29, 2017

@clintongormley

This comment has been minimized.

Member

clintongormley commented Jun 29, 2017

thought maybe we should make the default verbose=false for 6.x, but didn't reach a decision at the time.

we could do this quite cleanly - if no verbose flag is specified, then we can emit a deprecation warning saying they should use verbose=true going forwards, then change the default in 6.0

@colings86 colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment