Remove support for controversial `ignore_unavailable` and `allow_no_indices` from indices exists api #20712

javanna · 2016-09-30T16:06:33Z

Exist requests are supposed to never throw an exception, but rather return true or false depending on whether some resource exists or not. Indices exists does that for indices and accepts wildcard expressions. The way the api works internally is by resolving indices like any other api would do and catching IndexNotFoundException: if an exception is thrown false is returned, otherwise true. That works only if ignore_unavailable and allow_no_indices indices options are both set to false, meaning that they are strict and any missing index or wildcard expression that resolves to no indices leads to an exception that can be thrown and cause false to be returned.

Unfortunately the indices options have been configurable up until now for the indices exists request, meaning that one can set ignore_unavailable or allow_no_indices to true and have the indices exist request return true for indices that really don't exist, which makes very little sense in the context of this api.

For instance curl -iXHEAD localhost:9200/_all?allow_no_indices=true returns 200 OK against an empty cluster. allow_no_indices and ignore_unavailable affect badly what this api returns, hence I think they shouldn't be settable in this specific case.

This commit removes the indicesOptions setter from the IndicesExistsRequest and makes settable only expandWildcardsOpen and expandWildcardsClosed, hence a subset of the available indices options. This way we can guarantee more consistent behaviour of the indices exists api. ignore_unavailable will always be set to false, as well as allow_no_indices.

tlrx · 2016-10-03T08:49:47Z

rest-api-spec/src/main/resources/rest-api-spec/api/indices.exists.json

-        "allow_no_indices": {
-            "type" : "boolean",
-            "description" : "Whether to ignore if a wildcard indices expression resolves into no concrete indices. (This includes `_all` string or when no indices have been specified)"
-        },


I think we should document this a breaking change, just in case users rely on this broken behavior.

I see it differently. I think it is a bug and we help users that rely on it by fixing it. Otherwise non existing indices cause the api to return true with the "right" indices options, that really makes no sense to rely on I believe :)

I agree it makes no sense and I'm happy with this fix. I just think that a short line saying "params X and Y have been removed from the Indices Exists REST API" in changelog is still valuable. But as you want.

Yes that sounds good. It's just that if we treat it as a breaking change it can't go in a bugfix release, that is what I worry about :)

tlrx · 2016-10-03T08:52:59Z

...rc/main/java/org/elasticsearch/action/admin/indices/exists/indices/IndicesExistsRequest.java

+
+    public IndicesExistsRequest expandWilcardsClosed(boolean expandWildcardsClosed) {
+        this.indicesOptions = IndicesOptions.fromOptions(indicesOptions.ignoreUnavailable(), indicesOptions.allowNoIndices(),
+                indicesOptions.expandWildcardsOpen(), expandWildcardsClosed);


Maybe we could also check that ignore_unavailable and allow_no_indices are equals to false in the validate() method of the request?

why? You can't set them.

maybe we should do this in a test, given that if you can set those options something is broken. I will look into this.

tlrx · 2016-10-03T08:53:49Z

ignore_unavailable will always be set to true, as well as allow_no_indices.

Hum.. to falseinstead ?

I left some comments

javanna · 2016-10-03T09:20:42Z

ignore_unavailable will always be set to true, as well as allow_no_indices.
Hum.. to falseinstead ?

Right, I corrected that. Let me also ping @elastic/es-clients so they can tell us what they think about the change.

javanna · 2016-10-03T12:59:53Z

I chatted with the clients team and they are fine with this change, they also see it as a bugfix which means that it makes sense to backport it. @tlrx I will add some docs around the breaking change when backporting to 5.0 if that's ok with you.

epixa · 2016-10-04T21:08:50Z

Pardon my ignorance, but is this removing the ability to set ignore_unavailable at request time?

javanna · 2016-10-04T21:16:30Z

@epixa yes but only for indices exists api. Do you foresee this as a problem? Would love to hear why this "feature" would be needed for this specific api.

javanna · 2016-10-05T12:20:42Z

@epixa I updated the title to better reflect what this change is about. Could you please give me some feedback on whether this change could impact kibana? I thought kibana doesn't use this api at all, but I may be wrong. Thanks!

epixa · 2016-10-05T12:26:03Z

@javanna Thanks for the clarification. I don't think we use the Indices Exists api at all, but I just sent a message to the rest of the Kibana team to see if anyone can think of something that I'm missing. Unfortunately it's kind of hard to grep for this sort of thing in Kibana, so I can't determine if we use it statically.

javanna · 2016-10-05T12:32:53Z

we don't have many endpoints that support HEAD, maybe grepping HEAD would help, not sure :)

epixa · 2016-10-05T12:43:28Z

I think we're good on our end. Thanks for your patience!

…ces from indices exists api Exist requests are supposed to never throw an exception, but rather return true or false depending on whether some resource exists or not. Indices exists does that for indices and accepts wildcard expressions too. The way the api works internally is by resolving indices and catching IndexNotFoundException: if an exception is thrown the index does not exist hence it returns false, otherwise it returns true. That works ok only if ignore_unavailable and allow_no_indices indices options are both set to false, meaning that they are strict and any missing index or wildcard expressions that resolves to no indices will lead to an exception that can be thrown and cause false to be returned. Unfortunately the indices options have been configurable up until now for this request, meaning that one can set ignore_unavailable or allow_no_indices to true and have the indices exist request return true for indices that really don't exist, which makes very little sense in the context of this api. This commit removes the indicesOptions setter from the IndicesExistsRequest and makes settable only expandWildcardsOpen and expandWildcardsClosed, hence a subset of the available indices options. This way we can guarantee more consistent behaviour of the indices exists api. We can then remove the ignore_unavailable and allow_no_indices option from indices exists api spec

javanna · 2016-11-02T17:06:43Z

@tlrx I got back to this, can you have a final look please?

tlrx

LGTM, thanks @javanna

javanna · 2016-11-04T18:28:40Z

@clintongormley I didn't backport this as I saw you have removed 5.0.0 and 5.1.0 labels. Maybe we could consider it as a good enhancement for 5.1.0 though, let me know what you think.

javanna added >bug review :Data Management/Indices APIs APIs to create and manage indices and templates v6.0.0-alpha1 v5.1.1 v5.0.0 labels Sep 30, 2016

tlrx requested changes Oct 3, 2016

View reviewed changes

javanna force-pushed the fix/indices_exists_options branch from 2abeb1b to c953220 Compare October 3, 2016 13:06

javanna changed the title ~~Remove controversial indicesOptions setter from IndicesExistsRequest~~ Remove support for controversial ignore_unavailable and allow_no_indices from indices exists api Oct 4, 2016

clintongormley removed v5.0.0 v5.1.1 labels Oct 18, 2016

javanna force-pushed the fix/indices_exists_options branch from c953220 to ada5b89 Compare November 2, 2016 16:50

javanna force-pushed the fix/indices_exists_options branch from ada5b89 to 7120587 Compare November 2, 2016 17:05

javanna added >breaking >breaking-java >enhancement and removed >bug labels Nov 2, 2016

tlrx approved these changes Nov 4, 2016

View reviewed changes

javanna merged commit c2160a8 into elastic:master Nov 4, 2016

clintongormley removed the >enhancement label Nov 5, 2016

clintongormley changed the title ~~Remove support for controversial ignore_unavailable and allow_no_indices from indices exists api~~ Remove support for controversial ignore_unavailable and allow_no_indices from indices exists api May 5, 2017

javanna mentioned this pull request Dec 5, 2017

Incorrect REST API spec? #27681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove support for controversial `ignore_unavailable` and `allow_no_indices` from indices exists api #20712

Remove support for controversial `ignore_unavailable` and `allow_no_indices` from indices exists api #20712

javanna commented Sep 30, 2016 •

edited

Loading

tlrx Oct 3, 2016

javanna Oct 3, 2016

tlrx Oct 3, 2016

javanna Oct 3, 2016

tlrx Oct 3, 2016

javanna Oct 3, 2016

tlrx Oct 3, 2016

javanna Oct 3, 2016

tlrx commented Oct 3, 2016

javanna commented Oct 3, 2016

javanna commented Oct 3, 2016

epixa commented Oct 4, 2016

javanna commented Oct 4, 2016

javanna commented Oct 5, 2016

epixa commented Oct 5, 2016

javanna commented Oct 5, 2016

epixa commented Oct 5, 2016

javanna commented Nov 2, 2016

tlrx left a comment

javanna commented Nov 4, 2016

Remove support for controversial ignore_unavailable and allow_no_indices from indices exists api #20712

Remove support for controversial ignore_unavailable and allow_no_indices from indices exists api #20712

Conversation

javanna commented Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Oct 3, 2016

javanna commented Oct 3, 2016

javanna commented Oct 3, 2016

epixa commented Oct 4, 2016

javanna commented Oct 4, 2016

javanna commented Oct 5, 2016

epixa commented Oct 5, 2016

javanna commented Oct 5, 2016

epixa commented Oct 5, 2016

javanna commented Nov 2, 2016

tlrx left a comment

Choose a reason for hiding this comment

javanna commented Nov 4, 2016

Remove support for controversial `ignore_unavailable` and `allow_no_indices` from indices exists api #20712

Remove support for controversial `ignore_unavailable` and `allow_no_indices` from indices exists api #20712

javanna commented Sep 30, 2016 •

edited

Loading