Skip to content

Conversation

@martijnvg
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

@javanna
Copy link
Member

javanna commented Dec 23, 2014

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:

curl -XGET localhost:9200/index1/_search?ignore_unavailable=true

# fails if index1 is not there, although we said ignore_unavailable=true
{
  "error" : "IndexMissingException[[index1] missing]",
  "status" : 404
}

Is this something that we want to address as well?

@martijnvg
Copy link
Member Author

@javanna I updated the PR, addressed the minor comments and added tests for the case a single index mentioned doesn't exist.

@javanna
Copy link
Member

javanna commented Dec 23, 2014

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 ;)

@martijnvg martijnvg force-pushed the ignore_missing_closed_indices branch from 9d64cbb to 728c9a8 Compare December 23, 2014 18:12
…e index is specified in a search or broadcast request.

Closes elastic#9047
Closes elastic#7153
@martijnvg martijnvg force-pushed the ignore_missing_closed_indices branch from c30f55e to a345e98 Compare December 24, 2014 09:46
martijnvg added a commit that referenced this pull request Dec 24, 2014
…e index is specified in a search or broadcast request.

Closes #9047
Closes #7153
martijnvg added a commit that referenced this pull request Dec 24, 2014
…e index is specified in a search or broadcast request.

Closes #9047
Closes #7153
@martijnvg martijnvg merged commit a345e98 into elastic:master Dec 24, 2014
@zde
Copy link

zde commented Feb 12, 2015

This contradicts http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/multi-index.html
I see no rationale for special handling of the single non-wildcard index case.

@clintongormley clintongormley added :Core/Infra/Core Core issues without another label and removed review labels Mar 19, 2015
@martijnvg martijnvg deleted the ignore_missing_closed_indices branch May 18, 2015 23:28
@clintongormley clintongormley changed the title Core: ignore_unavailable shouldn't ignore closed indices ignore_unavailable shouldn't ignore closed indices Jun 7, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…e index is specified in a search or broadcast request.

Closes elastic#9047
Closes elastic#7153
javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 21, 2016
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
javanna added a commit that referenced this pull request Nov 21, 2016
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
javanna added a commit that referenced this pull request Nov 21, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants