The `ignore_unavailable` option should also ignore indices that are closed #6475

martijnvg · 2014-06-12T09:06:32Z

The ignore_unavailable option should also ignore indices that are closed, but it doesn't at the moment. The PR makes sure that ignore_unavailable also ignores specified indices that are closed.

PR for #6471

javanna · 2014-06-12T09:28:42Z

src/main/java/org/elasticsearch/cluster/metadata/MetaData.java


+    public String[] concreteIndices(IndicesOptions indicesOptions, boolean canFailClosed, String... aliasesOrIndices) throws IndexMissingException, ElasticsearchIllegalArgumentException {


Can we avoid having different public concreteIndices variations? I'd love it if the new flag was part of the IndicesOptions...

The 'ignore_unavailable' option should cover both closed and missing indices, but there is a difference between apis. For example search, count and percolate throw cluster block exception when an index is closed, while update setting, mappings etc allow to be executed on a closed index (in fact certain settings can only be changed on a closed index). This difference isn't expressed in IndicesOptions, we can add it, but IMO it shouldn't be exposed as a setting (apis either never ignore closed indices or do ignore/fail closed indices).

I see your point, on the other hand I think we can have options that are part of IndicesOptions although not exposed to users, even if they change only depending on the api. That was the direction in #6169. It would be great if we can keep a single concreteIndices method that knows what to do based on the IndicesOptions provided as argument and move this canFailClosed flag to IndicesOptions without exposing it to users.

+1 I'll update the PR.

On 12 June 2014 12:38, Luca Cavanna notifications@github.com wrote:

In src/main/java/org/elasticsearch/cluster/metadata/MetaData.java:

public String[] concreteIndices(IndicesOptions indicesOptions, boolean canFailClosed, String... aliasesOrIndices) throws IndexMissingException, ElasticsearchIllegalArgumentException {

I see your point, on the other hand I think we can have options that are
part of IndicesOptions although not exposed to users, even if they change
only depending on the api. That was the direction in #6169
#6169. It would be
great if we can keep a single concreteIndices method that knows what to
do based on the IndicesOptions provided as argument and move this
canFailClosed flag to IndicesOptions without exposing it to users.

—
Reply to this email directly or view it on GitHub
https://github.com/elasticsearch/elasticsearch/pull/6475/files#r13695415
.

Met vriendelijke groet,

Martijn van Groningen

martijnvg · 2014-06-17T09:17:09Z

@javanna I updated the PR and moved the 'canFailClosed' to IgnoreIndices.

javanna · 2014-06-17T15:17:02Z

src/main/java/org/elasticsearch/action/percolate/MultiPercolateRequest.java

-        boolean allowNoIndices = IndicesOptions.strictExpandOpen().allowNoIndices();
-        boolean expandWildcardsOpen = IndicesOptions.strictExpandOpen().expandWildcardsOpen();
-        boolean expandWildcardsClosed = IndicesOptions.strictExpandOpen().expandWildcardsClosed();
+        IndicesOptions defaultOptions = indicesOptions;


not sure if you wanted to do the following instead?

IndicesOptions defaultOptions = IndicesOptions.strictExpandOpenAndForbidClosed();

The indicesOptions field is already set to IndicesOptions.strictExpandOpenAndForbidClosed(), so this should be ok?

javanna · 2014-06-17T15:59:49Z

Left a few comments, looks good though

martijnvg · 2014-06-20T21:02:54Z

Thanks @javanna, I updated the PR.

javanna · 2014-06-30T18:48:07Z

src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

+                return actualIndices.toArray(new String[actualIndices.size()]);
+            } else {
+                return aliasesOrIndices;
+            }


This code block is an optimization that's useful in case there's no aliases in the input argument and we can return that same array quickly as we received it. I wonder if it still makes sense to have this optimization given that we might need to modify the array. I would love to remove it as it adds complexity... What do you think @martijnvg?

Most of the times requested indices aren't closed indices, so I think in that case the additional checks is worth it.

javanna · 2014-06-30T18:49:24Z

Left a small comment, LGTM otherwise

martijnvg · 2014-07-01T10:09:16Z

@javanna Thanks for reviewing it!

…re closed. Closes elastic#6471 Closes elastic#6475

…re closed. Closes #6471 Closes #6475

javanna · 2014-07-10T12:25:29Z

Side note: this change makes it possible to ignore closed indices when using the ignore_unavailable option, whereas we would previously return a ClusterBlockException. It also changes the error returned when searching against closed indices without ignore_unavailable set, which is not a ClusterBlockException anymore but the newly introduced IndexClosedException.

…tion on closed indices Single index operations to use the newly added IndexClosedException introduced with elastic#6475. This way we can also fail faster when we are trying to execute operations on closed indices and their use is not allowed (depending on indices options). Indices blocks are still checked but we can already throw error while resolving indices (MetaData#concreteIndices). Effectively this change also affects what we return when using one of the following apis: analyze, bulk, index, update, delete, explain, get, multi_get, mlt, term vector, multi_term vector. We now return `{"error":"IndexClosedException[[test] closed]","status":403}` instead of `{"error":"ClusterBlockException[blocked by: [FORBIDDEN/4/index closed];]","status":403}`. Closes elastic#6988

…tion on closed indices Single index operations to use the newly added IndexClosedException introduced with #6475. This way we can also fail faster when we are trying to execute operations on closed indices and their use is not allowed (depending on indices options). Indices blocks are still checked but we can already throw error while resolving indices (MetaData#concreteIndices). Effectively this change also affects what we return when using one of the following apis: analyze, bulk, index, update, delete, explain, get, multi_get, mlt, term vector, multi_term vector. We now return `{"error":"IndexClosedException[[test] closed]","status":403}` instead of `{"error":"ClusterBlockException[blocked by: [FORBIDDEN/4/index closed];]","status":403}`. Closes #6988

…re closed. Closes elastic#6471 Closes elastic#6475

martijnvg added the review label Jun 12, 2014

javanna reviewed Jun 12, 2014
View reviewed changes

javanna reviewed Jun 17, 2014
View reviewed changes

s1monw removed the review label Jun 18, 2014

javanna reviewed Jun 30, 2014
View reviewed changes

Core: The ignore_unavailable=true setting also ignores indices that a…

c27b041

…re closed. Closes elastic#6471 Closes elastic#6475

martijnvg added a commit that referenced this pull request Jul 1, 2014

Core: The ignore_unavailable=true setting also ignores indices that a…

b9fe169

…re closed. Closes #6471 Closes #6475

martijnvg closed this in 85bea22 Jul 1, 2014

martijnvg added a commit that referenced this pull request Jul 1, 2014

Core: The ignore_unavailable=true setting also ignores indices that a…

6f9aa24

…re closed. Closes #6471 Closes #6475

martijnvg changed the title ~~The ignore_unavailable option should also ignore indices that are closed~~ Core: The ignore_unavailable option should also ignore indices that are closed Jul 2, 2014

martijnvg added bug labels Jul 2, 2014

javanna mentioned this pull request Jul 23, 2014

Internal: streamline use of IndexClosedException #6988

Closed

javanna mentioned this pull request Jul 23, 2014

Streamline use of IndexClosedException introduced with #6475 #6990

Merged

This was referenced Dec 23, 2014

ignore_unavailable shouldn't ignore closed indices #9047

Merged

Verify the index state of concrete indices after alias resolution #9057

Merged

martijnvg deleted the bug/ignore_unavailable_not_working_for_closed_indices branch May 18, 2015 23:31

clintongormley added the :Internal label Jun 7, 2015

clintongormley changed the title ~~Core: The ignore_unavailable option should also ignore indices that are closed~~ The ignore_unavailable option should also ignore indices that are closed Jun 7, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

Core: The ignore_unavailable=true setting also ignores indices that a…

f7046dc

…re closed. Closes elastic#6471 Closes elastic#6475

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The `ignore_unavailable` option should also ignore indices that are closed #6475

The `ignore_unavailable` option should also ignore indices that are closed #6475

martijnvg commented Jun 12, 2014

javanna Jun 12, 2014

martijnvg Jun 12, 2014

javanna Jun 12, 2014

martijnvg Jun 12, 2014

martijnvg commented Jun 17, 2014

javanna Jun 17, 2014

martijnvg Jun 20, 2014

javanna commented Jun 17, 2014

martijnvg commented Jun 20, 2014

javanna Jun 30, 2014

martijnvg Jul 1, 2014

javanna commented Jun 30, 2014

martijnvg commented Jul 1, 2014

javanna commented Jul 10, 2014


		public String[] concreteIndices(IndicesOptions indicesOptions, boolean canFailClosed, String... aliasesOrIndices) throws IndexMissingException, ElasticsearchIllegalArgumentException {

The ignore_unavailable option should also ignore indices that are closed #6475

The ignore_unavailable option should also ignore indices that are closed #6475

Conversation

martijnvg commented Jun 12, 2014

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

martijnvg commented Jun 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jun 17, 2014

martijnvg commented Jun 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jun 30, 2014

martijnvg commented Jul 1, 2014

javanna commented Jul 10, 2014

The `ignore_unavailable` option should also ignore indices that are closed #6475

The `ignore_unavailable` option should also ignore indices that are closed #6475