Unified MetaData#concreteIndices methods into a single method that accepts indices (or aliases) and indices options #6169
Conversation
…cepts indices (or aliases) and indices options Added new internal flag to IndicesOptions that tells whether aliases can be resolved to multiple indices or not.
| @@ -19,6 +19,7 @@ | |||
| package org.elasticsearch.action.support; | |||
There was a problem hiding this comment.
First thing first... lets move all the flags to named constants... cause it's really becoming hard to follow
|
Left a few comments, overall great direction. While you're at it, would be nice to have a builder for the |
| } | ||
| aliasesOrIndices = convertFromWildcards(aliasesOrIndices, indicesOptions); | ||
|
|
||
| // optimize for single element index (common case) | ||
| if (aliasesOrIndices.length == 1) { |
There was a problem hiding this comment.
@uboness wondering if we still need this optimization, removing doesn't change anything, does it really help performance wise???
…or all the api previously using MetaData#concreteIndices(String[], IndicesOptions) Removed old unused method, deprecation is not needed as it doesn't break client code.
Left it as a shortcut although it calls the common concreteIndices that accepts IndicesOptions and multipleIndices
|
I just pushed a few more commits that address the comments and finalize the refactoring. |
| public String concreteIndex(String index) throws IndexMissingException, ElasticsearchIllegalArgumentException { | ||
| public String concreteSingleIndex(String indexOrAlias) throws IndexMissingException, ElasticsearchIllegalArgumentException { | ||
| String[] indices = concreteIndices(IndicesOptions.strictSingleIndexNoExpand(), indexOrAlias); | ||
| assert indices.length == 1; |
There was a problem hiding this comment.
should this be a real exception? I mean it's clearly wrong?
There was a problem hiding this comment.
yeah... agree... an assert does't make much sense here... in fact, I'd consider removing this method all together and move the check to the api that requires a single index (so the exception message will have the right context)
There was a problem hiding this comment.
both good points... the reason why it's an assert is that it should never happen, as we pass in a single index and we say we don't resolve aliases to multiple indices. Can make it an exception but then it would seem that we handle the case while this must not happen unless there are bugs.
As for removing the method entirely. I like the idea, but then we'd end up with these same two lines all over the place...not sure it's great, I thought it's better to share those lines (length check and array to single string conversion).
There was a problem hiding this comment.
both good points... the reason why it's an assert is that it should never happen, as we pass in a single index and we say we don't resolve aliases to multiple indices.
but an alias to a single index is allowed? If "yes", then it needs to be an exception, as it's not under the code control what input the user provides (the user can provide an alias that points to multiple indices... )
As for removing the method entirely. I like the idea, but then we'd end up with these same two lines all over the place...not sure it's great, I thought it's better to share those lines (length check and array to single string conversion).
fair point... if it's called all over the place, then perhaps the exception should be thrown here (with somewhat of a generic yet descriptive & user friendly message)
There was a problem hiding this comment.
yes an alias to a single index is allowed. but I wasn't too clear... when I said we don't resolve aliases to multiple indices (with these indices options) I meant that we throw an exception in that case! Thus the case where length != 1 never happens whatever the user input is...cause we throw an exception in all other cases with those specific indices options.
There was a problem hiding this comment.
ah.. ok, gotcha... I'd leave the assert here but change it to
assert indices.length == 1 : "expected an exception to be thrown otherwise";
for clarity
|
LGTM |
| if (allowAliasesToMultipleIndices() || out.getVersion().onOrAfter(Version.V_1_2_0)) { | ||
| out.write(id); | ||
| } else { | ||
| out.write(id ^ DONT_ALLOW_ALIASES_TO_MULTIPLE_INDICES); |
There was a problem hiding this comment.
I think we should do this differently we want to flip all the bits to 0 that are > 4th bit you should do id & 0xF
There was a problem hiding this comment.
Right, if we were to add other flags, the XOR would only flip the 5th bit...
|
Merged |
clintongormley
added
the
:Data Management/Indices APIs
The goal of this PR is to have a single method in
MetaDatathat allows to resolve aliases or indices (eventually containing wildcard expressions) to concrete indices. That way all the apis will just refer to this same new method passing the proper indices options as argument.In order to achieve that a new internal flag has been added to IndicesOptions, which tells whether aliases can be resolved to multiple indices or not.
Also it's now possible to decide not to expand wildcards at all via indices options, while previously it was possible to decide whether to expand them to open indices, closed indices, or both.
Note that the PR is against 1.x to handle backwards compatibility properly when needed. Bw comp checks will be removed when porting to master.