Skip to content
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

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

Conversation

Projects
None yet
4 participants
@martijnvg
Copy link
Member

martijnvg commented Jun 12, 2014

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

@martijnvg martijnvg added the review label Jun 12, 2014

@javanna

View changes

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

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

This comment has been minimized.

Copy link
@javanna

javanna Jun 12, 2014

Member

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

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jun 12, 2014

Author Member

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

This comment has been minimized.

Copy link
@javanna

javanna Jun 12, 2014

Member

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.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jun 12, 2014

Author Member

+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

This comment has been minimized.

Copy link
Member Author

martijnvg commented Jun 17, 2014

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

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

This comment has been minimized.

Copy link
@javanna

javanna Jun 17, 2014

Member

not sure if you wanted to do the following instead?

IndicesOptions defaultOptions = IndicesOptions.strictExpandOpenAndForbidClosed();

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jun 20, 2014

Author Member

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

@javanna

View changes

src/main/java/org/elasticsearch/action/support/IndicesOptions.java Outdated
out.write(id);
} else if (allowAliasesToMultipleIndices() || out.getVersion().onOrAfter(Version.V_1_2_0)) {

This comment has been minimized.

Copy link
@javanna

javanna Jun 17, 2014

Member

I'd probably remove the needless allowAliasesToMultipleIndices() to make these ifs clearer. I'd also update the comment below and add a small comment here on why we are doing this.

@javanna

View changes

src/main/java/org/elasticsearch/indices/IndexClosedException.java Outdated
import org.elasticsearch.rest.RestStatus;

/**
*

This comment has been minimized.

Copy link
@javanna

javanna Jun 17, 2014

Member

can we add some javadocs?

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jun 17, 2014

Left a few comments, looks good though

@s1monw s1monw removed the review label Jun 18, 2014

@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Jun 20, 2014

Thanks @javanna, I updated the PR.

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

This comment has been minimized.

Copy link
@javanna

javanna Jun 30, 2014

Member

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?

This comment has been minimized.

Copy link
@martijnvg

martijnvg Jul 1, 2014

Author Member

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

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jun 30, 2014

Left a small comment, LGTM otherwise

@martijnvg

This comment has been minimized.

Copy link
Member Author

martijnvg commented Jul 1, 2014

@javanna Thanks for reviewing it!

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

@martijnvg martijnvg closed this in 85bea22 Jul 1, 2014

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

@martijnvg 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 martijnvg added bug labels Jul 2, 2014

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jul 10, 2014

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.

javanna added a commit to javanna/elasticsearch that referenced this pull request Jul 24, 2014

Internal: streamline use of IndexClosedException when executing opera…
…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

javanna added a commit that referenced this pull request Jul 24, 2014

Internal: streamline use of IndexClosedException when executing opera…
…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

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

@clintongormley 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.