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

Fix default value of ignore_unavailable for snapshot REST API (#25359) #27056

Merged
merged 3 commits into from
Nov 16, 2017
Merged

Fix default value of ignore_unavailable for snapshot REST API (#25359) #27056

merged 3 commits into from
Nov 16, 2017

Conversation

liketic
Copy link

@liketic liketic commented Oct 20, 2017 •
edited by ywelsch
Loading

The default value for ignore_unavailable did not match what was documented when using the REST APIs for snapshot creation and restore. This PR sets the default value of ignore_unavailable to false, the way it is documented and ensures it's the same when using either REST API or transport client.

Closes #25359

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@liketic
Copy link
Author

liketic commented Oct 24, 2017

@abeyad Could you please review this? Thanks in advance.

@dnhatn
Copy link
Member

dnhatn commented Oct 24, 2017

@elasticmachine please test this.

@liketic liketic force-pushed the bugfix/issues/25359 branch from 077b167 to 1694583 Compare October 25, 2017 04:56
@liketic
Copy link
Author

liketic commented Oct 25, 2017

@dnhatn I rebased and get test passed. Please test again.

@tvernum
Copy link
Contributor

tvernum commented Oct 25, 2017

@elasticmachine ok to test

ywelsch
ywelsch suggested changes Nov 15, 2017
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Indeed, the REST API is not aligned with the defaults of the transport client. I have made a suggestion that makes sure they're always aligned. Can you also fix RestoreSnapshotRequest?

...main/java/org/elasticsearch/action/admin/cluster/snapshots/create/CreateSnapshotRequest.java Outdated
@@ -402,7 +403,7 @@ public CreateSnapshotRequest source(Map<String, Object> source) {
includeGlobalState = nodeBooleanValue(entry.getValue(), "include_global_state");
}
}
indicesOptions(IndicesOptions.fromMap((Map<String, Object>) source, IndicesOptions.lenientExpandOpen()));
indicesOptions(IndicesOptions.fromMap(source, IndicesOptions.strictExpandOpen()));
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 this should be fixed by falling back to indicesOptions instead (which is equal to IndicesOptions.strictExpandOpen() by default)

@liketic
Copy link
Author

liketic commented Nov 16, 2017

@ywelsch Thanks for reviewing this. I pushed 9c657d7

@ywelsch ywelsch added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >bug v6.0.1 v6.1.0 v7.0.0 labels Nov 16, 2017
@ywelsch
Copy link
Contributor

ywelsch commented Nov 16, 2017

Thanks for the quick update. Can you also add a REST test in rest-api-spec/test/snapshot.create/10_basic.yml that checks that creating a snapshot with an unavailable index name fails? (i.e. that the defaults match what's in the documentation). Note that this will require adding a skip section (for tests in mixed clusters with older versions):

  - skip:
      version: " - 6.99.99"

For now, you can specify "-6.99.99", I will take care about changing it once we decide on backporting this PR.

Finally can you update the PR title and description? Thanks a lot!

@liketic liketic changed the title Fix creating snapshot on missing index (#25359) Fix default value of ignore_unavailable for snapshot REST API (#25359) Nov 16, 2017
@liketic
Copy link
Author

liketic commented Nov 16, 2017

@ywelsch Thanks. I updated the REST test. Hope it makes sense.

@ywelsch
Copy link
Contributor

ywelsch commented Nov 16, 2017

@elasticmachine retest this please

ywelsch
ywelsch approved these changes Nov 16, 2017
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

Thanks again @liketic. I took the liberty of editing the PR description and will merge this soon.

@ywelsch ywelsch merged commit 6b81748 into elastic:master Nov 16, 2017
ywelsch pushed a commit that referenced this pull request Nov 16, 2017
@ywelsch
Fix default value of ignore_unavailable for snapshot REST API (#27056)
cd69a9e 
The default value for ignore_unavailable did not match what was documented when using the REST APIs for snapshot creation and restore. This commit sets the default value of ignore_unavailable to false, the way it is documented and ensures it's the same when using either REST API or transport client.

Closes #25359
ywelsch pushed a commit that referenced this pull request Nov 16, 2017
@ywelsch
Fix default value of ignore_unavailable for snapshot REST API (#27056)
9073cc6 
The default value for ignore_unavailable did not match what was documented when using the REST APIs for snapshot creation and restore. This commit sets the default value of ignore_unavailable to false, the way it is documented and ensures it's the same when using either REST API or transport client.

Closes #25359
ywelsch added a commit that referenced this pull request Nov 16, 2017
@ywelsch
Stop skipping REST test after backport of #27056
3b963dc
@liketic liketic deleted the bugfix/issues/25359 branch November 16, 2017 15:13
jasontedor added a commit that referenced this pull request Nov 16, 2017
@jasontedor
Merge branch 'master' into ccr
c1e2257 
* master:
  Stop skipping REST test after backport of #27056
  Fix default value of ignore_unavailable for snapshot REST API (#27056)
  Add composite aggregator (#26800)
  Fix `ShardSplittingQuery` to respect nested documents. (#27398)
  [Docs] Restore section about multi-level parent/child relation in parent-join (#27392)
  Add TcpChannel to unify Transport implementations (#27132)
  Add note on plugin distributions in plugins folder
  Remove implementations of `TransportChannel` (#27388)
  Update Google SDK to version 1.23 (#27381)
  Fix Gradle 4.3.1 compatibility for logging (#27382)
  [Test] Change Elasticsearch startup timeout to 120s in packaging tests
  Docs/windows installer (#27369)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywelsch ywelsch ywelsch approved these changes

Assignees
No one assigned
Labels
>bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v6.0.1 v6.1.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@liketic @elasticmachine @dnhatn @tvernum @ywelsch @colings86