-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
ignore_unavailable
shouldn't ignore closed indices
#9047
ignore_unavailable
shouldn't ignore closed indices
#9047
Conversation
* @param indicesOptions the indices options to be used for the index resolution | ||
* @return the concrete index obtained as a result of the index resolution | ||
* @throws IndexMissingException if the index or alias provided doesn't exist | ||
* @throws IndexMissingException if the index or alias provided doesn't exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
throw new ElasticsearchIllegalArgumentException("Alias [" + aliasOrIndex + "] has more than one indices associated with it [" + Arrays.toString(indices) + "], can't execute a single index op"); | ||
} | ||
|
||
indexMetaData = this.indices.get(aliasOrIndex); | ||
if (indexMetaData != null && indexMetaData.getState() == IndexMetaData.State.CLOSE && failClosed) { | ||
throw new IndexClosedException(new Index(aliasOrIndex)); | ||
if (indexMetaData != null && indexMetaData.getState() == IndexMetaData.State.CLOSE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remind me why we need this check again? Didn't we do it at the beginning of the method already? Maybe this if
was a leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the index the alias is pointing to may be in a closed state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this if
completely, cause we call the same method given the same input at the beginning of the method, this condition will never be true here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I did some digging, this if was introduced with #6475, and I think its purpose was to check that state of indices after aliases resolution. This is a bug that we should address on a separate issue, that should be about index state checks when resolving aliases, since it seems we don't check that at all at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, lets open another issue for this.
I left a couple of minor comments @martijnvg . The one thing I am not sure about is that the PR addresses the case of closed indices, while the related issue might be referring to more usecases around the single index special case, like:
Is this something that we want to address as well? |
@javanna I updated the PR, addressed the minor comments and added tests for the case a single index mentioned doesn't exist. |
Thanks @mvg LGTM, the single missing index case I mentioned before was something I overlooked on my end, it works as expected already, so this PR addresses what it should addresses, sorry for the confusion ;) |
9d64cbb
to
728c9a8
Compare
…e index is specified in a search or broadcast request. Closes elastic#9047 Closes elastic#7153
c30f55e
to
a345e98
Compare
This contradicts http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/multi-index.html |
ignore_unavailable
shouldn't ignore closed indicesignore_unavailable
shouldn't ignore closed indices
…e index is specified in a search or broadcast request. Closes elastic#9047 Closes elastic#7153
This is a followup to elastic#21689 where we removed a misplaced try catch for IndexMissingException and IndexClosedException which was related to elastic#9047 (at least for the index closed case). The code block within the change was moved as part of elastic#20890, which made the catch redundant. It was somehow used before (e.g. in 5.0) but it doesn't seem that this catch had any effect. Added tests to verify that. In fact a specific catch added to the search api only would defeat the purpose of having common indices options that work throughout all our APIs. Relates to elastic#21689
This is a followup to #21689 where we removed a misplaced try catch for IndexMissingException and IndexClosedException which was related to #9047 (at least for the index closed case). The code block within the change was moved as part of #20890, which made the catch redundant. It was somehow used before (e.g. in 5.0) but it doesn't seem that this catch had any effect. Added tests to verify that. In fact a specific catch added to the search api only would defeat the purpose of having common indices options that work throughout all our APIs. Relates to #21689
This is a followup to #21689 where we removed a misplaced try catch for IndexMissingException and IndexClosedException which was related to #9047 (at least for the index closed case). The code block within the change was moved as part of #20890, which made the catch redundant. It was somehow used before (e.g. in 5.0) but it doesn't seem that this catch had any effect. Added tests to verify that. In fact a specific catch added to the search api only would defeat the purpose of having common indices options that work throughout all our APIs. Relates to #21689
The
ignore_unavailable
request setting shouldn't ignore closed indices if a single index is specified in a search or broadcast request.PR for #7153