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

martijnvg · 2014-12-24T11:16:27Z

No description provided.

javanna · 2014-12-24T11:19:29Z

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

@@ -760,24 +760,44 @@ public String concreteSingleIndex(String indexOrAlias, IndicesOptions indicesOpt
        }
        // not an actual index, fetch from an alias
        String[] indices = aliasAndIndexToIndexMap.getOrDefault(aliasOrIndex, Strings.EMPTY_ARRAY);
-        if (indices.length == 0 && !allowNoIndices) {
+        if (indices.length == 0 && !failNoIndices) {


I think you should remove the ! here, throw exception if failNoIndices is true

Are we sure we wanna rename allowNoIndices to failNoIndices? Wondering if t becomes more error-prone than before :)

javanna · 2014-12-24T11:49:08Z

Left a couple of comments. I think the important bit here is to highlight that we didn't check indices state at all after expanding aliases. With this PR we are introducing an additional check that honours the ignore_unavailable option, meaning that if an alias expands to multiple indices, closed indices will be ignored or cause an exception depending on the value of ignore_unavailable. Want to double check this @clintongormley ? I wonder if it's ok to execute an operation on only a subset of indices that an alias points to...

clintongormley · 2014-12-24T12:01:30Z

I wonder if it's ok to execute an operation on only a subset of indices that an alias points to...

So the process is: resolve aliases to indices, then do the closed/open check, then execute the operation.

I think that, if the user has specified ignore_unavailable then they're expecting the operation to execute only on available indices, so it seems OK to me, no? Any ideas of when it might not be OK?

javanna · 2014-12-24T12:18:18Z

Any ideas of when it might not be OK?

I tend to agree that this is the expected behaviour, wondering because aliases are different from wildcard expressions, meaning that users might not know they are using an alias, and depending on the state of the underlying indices we end up executing an action against different indices, although the same alias is provided. That said we have the ignore_unavailable option to control this, so we should be fine.

javanna · 2014-12-24T12:34:36Z

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

-            }
+                return indices;
+            default:
+                ObjectOpenHashSet<String> concreteIndices = ObjectOpenHashSet.from(indices);


wondering if the other way around would be better: create a new array instead of set and add open indices to it from within the loop.

ok, so a new array and only open indices are added to that from the loop?

yes, that's what I was thinking, I'd maybe check state != CLOSE rather than checking for OPEN, but that's a super minor concern that doesn't change the result at this time

javanna · 2014-12-24T14:13:02Z

One more thing, let's discuss if this should be treated as a bug and pushed to 1.4.

Before this fix, operations against aliases didn't check the indices state. Thus operations that can't be executed against closed indices (e.g. search) will return a cluster block all the time:

{"error":"ClusterBlockException[blocked by: [FORBIDDEN/4/index closed];]","status":403}

After the fix, depending on ignore_unavailable we will either filter out the closed indices during aliases resolution, hence the operation will be executed against the resolved open indices only, or we'll return the following:

{"error":"IndexClosedException[[alias] closed]","status":403}

Looking back this completes what we did in #6475 and #6990 and makes the behaviour consistent when using aliases as well.

As for backwards compatibility, we would just return a better error (http response code stays the same) for operations that don't ignore unavailable indices, and we honour the ignore_unavailable option. I'm leaning towards treating it as a bugfix and push it to 1.4 too, the previous behaviour is a bug.

javanna · 2014-12-24T14:14:41Z

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

@@ -760,24 +760,49 @@ public String concreteSingleIndex(String indexOrAlias, IndicesOptions indicesOpt
        }
        // not an actual index, fetch from an alias
        String[] indices = aliasAndIndexToIndexMap.getOrDefault(aliasOrIndex, Strings.EMPTY_ARRAY);
-        if (indices.length == 0 && !allowNoIndices) {
+        if (indices.length == 0 && failNoIndices) {


thinking... in javadocs and probably es docs as well, we always refer to allowNoIndices and ignore_unavailable as options that affect how wildcard expansion works. But those options also affects how aliases expansion to concrete indices work. I think we should double check docs and update it.

Can you double check the javadocs and es docs around indices options? Those options affect now not only how wildcards get expanded, but also how aliases get resolved to concrete indices.

martijnvg · 2014-12-24T16:49:29Z

Ok, lets also push it to 1.4 branch for the reasons you mentioned. I think that:

"IndexClosedException[[alias] closed]"

message is good enough and consistent to when a concrete index is closed. We just need to mention the alias name.

javanna · 2015-01-07T12:40:05Z

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

+                            }
+                        } else if (indexMetaData.getState() == IndexMetaData.State.OPEN) {
+                            concreteIndices.add(index);
+                        }


nitpicking: maybe add an else that throws exception (index state not supported?)

javanna · 2015-01-07T12:41:08Z

Hey @martijnvg I left a couple of comments around docs and the returned error message, if you can address those then this is ready to be merged ;)

martijnvg · 2015-01-07T14:30:35Z

@javanna I've updated the PR.

javanna · 2015-01-07T15:25:04Z

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

+                    if (indexMetaData != null) {
+                        if (indexMetaData.getState() == IndexMetaData.State.CLOSE) {
+                            if (failClosed) {
+                                throw new IndexClosedException(new Index(aliasOrIndex));


name needs to be replaced here...that is why I don't like these optimizations :) they make the code error-prone ;)

argg... i'll change it :)

javanna · 2015-01-07T15:55:42Z

LGTM

…osed and deal with this according to IndicesOptions. Closes elastic#9057

…osed and deal with this according to IndicesOptions. Closes #9057

…osed and deal with this according to IndicesOptions. Closes elastic#9057

martijnvg added review >bug v1.4.3 v1.5.0 v2.0.0-beta1 labels Dec 24, 2014

javanna reviewed Dec 24, 2014
View reviewed changes

javanna reviewed Jan 7, 2015
View reviewed changes

martijnvg force-pushed the check_state_after_alias_resolution branch from 4c7427b to 5a46b67 Compare January 7, 2015 14:30

javanna reviewed Jan 7, 2015
View reviewed changes

martijnvg removed the review label Jan 8, 2015

martijnvg added 2 commits January 8, 2015 16:43

Renamed allowNoIndices to failNoIndices and changed parameter order.

b0b61ee

Core: Also check if indices resolved via aliases resolution aren't cl…

dedaf93

…osed and deal with this according to IndicesOptions. Closes elastic#9057

martijnvg force-pushed the check_state_after_alias_resolution branch from 580b802 to dedaf93 Compare January 8, 2015 15:47

martijnvg added a commit that referenced this pull request Jan 8, 2015

Core: Also check if indices resolved via aliases resolution aren't cl…

e762f22

…osed and deal with this according to IndicesOptions. Closes #9057

martijnvg merged commit dedaf93 into elastic:master Jan 8, 2015

martijnvg added a commit that referenced this pull request Jan 8, 2015

Core: Also check if indices resolved via aliases resolution aren't cl…

d1f317c

…osed and deal with this according to IndicesOptions. Closes #9057

clintongormley mentioned this pull request Feb 9, 2015

NullPointerException while bulk insert via alias on deleted index #9580

Closed

clintongormley added the :Core/Infra/Core Core issues without another label label Mar 19, 2015

javanna mentioned this pull request Apr 15, 2015

expand_wildcards:closed does not work to indices with alias #10591

Closed

martijnvg deleted the check_state_after_alias_resolution branch May 18, 2015 23:27

clintongormley changed the title ~~Core: Verify the index state of concrete indices after alias resolution~~ Verify the index state of concrete indices after alias resolution Jun 7, 2015

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

Core: Also check if indices resolved via aliases resolution aren't cl…

c97fc27

…osed and deal with this according to IndicesOptions. Closes elastic#9057

javanna mentioned this pull request Oct 15, 2015

Searching multiple aliases using wildcard and ignore_unavailable=true throws index closed exception #13278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

martijnvg commented Dec 24, 2014

javanna Dec 24, 2014

javanna Dec 24, 2014

javanna commented Dec 24, 2014

clintongormley commented Dec 24, 2014

javanna commented Dec 24, 2014

javanna Dec 24, 2014

martijnvg Dec 24, 2014

javanna Dec 24, 2014

javanna commented Dec 24, 2014

javanna Dec 24, 2014

javanna Jan 7, 2015

martijnvg commented Dec 24, 2014

javanna Jan 7, 2015

javanna commented Jan 7, 2015

martijnvg commented Jan 7, 2015

javanna Jan 7, 2015

martijnvg Jan 7, 2015

javanna commented Jan 7, 2015

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

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

Conversation

martijnvg commented Dec 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Dec 24, 2014

clintongormley commented Dec 24, 2014

javanna commented Dec 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Dec 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Dec 24, 2014

Choose a reason for hiding this comment

javanna commented Jan 7, 2015

martijnvg commented Jan 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jan 7, 2015