Refactor MetaData to split off the concrete index name logic to a dedicated service #12058

martijnvg · 2015-07-06T15:37:12Z

Changes in a nutshell:

All expression logic is now encapsulated by ExpressionResolver interface.
MetaData#convertFromWildcards() gets replaced by WildcardExpressionResolver.
All of the indices expansion methods are being moved from MetaData class to the new IndexNameExpressionResolver class.
All single index expansion optimisations are removed.

The logic for resolving a concrete index name from an expression has been moved from the MetaData class to IndexNameExpressionResolver class. The logic has been cleaned up and simplified were was possible without breaking bwc. Mainly the logic that deals if there is just one index expression is no more, mainly to make to code simpler. Also the notion of aliasOrIndex has been changed to index expression.

The IndexNameExpressionResolver translates index name expressions into concrete indices. The list of index name expressions are first delegated to the known IndexExpressionFilter. An IndexExpressionFilter is responsible for translating if possible an expression into a concrete indices or aliases otherwise the expression is left untouched. Concretely this means converting wildcard expressions into concrete indices or aliases, but in the future other implementations could convert expressions based on different rules.

Beyond this there is also some follow up work to do:

Replace SnapshotUtils#filterIndices by using the IndexNameExpressionResolver
During the lifetime of a request index expressions or resolved to concrete indices multiple times. (finding the search routing, whether aliases have filters, checking for cluster blocks and just resolving the index name expressions to concrete indices) This can be reduced to a single lookup.
Revise the in memory data structures used by MetaData. This is interesting in the case of many indices and aliases. For example I think that MetaData#aliasAndIndexToIndexMap can be removed in order to save heap memory at the cost of some extra look ups at request time. Also a completely different approach can be taken to store index and aliases with their respective IndexMetaData and AliasMetaData into a FST.

javanna · 2015-07-09T13:05:04Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

+        // This must be addressed in a follow up issue
+        if (indexExpressions.length == 1) {
+            failNoIndices = options.allowNoIndices() == false;
+        }


I did quite some digging around this special case. It is there from the early days of indices options and back when I added unit tests for it to MetaDataTests I probably just added tests to verify the current behaviour (assuming that what we had was ok). Looking at it more I don't get this test that fails when you remove this special case:

IndicesOptions noExpandErrorUnavailable = IndicesOptions.fromOptions(false, true, false, false); String[] results = md.concreteIndices(noExpandErrorUnavailable, "baz*"); assertThat(results, emptyArray()); try { md.concreteIndices(noExpandErrorUnavailable, "foo", "baz*"); fail(); } catch (IndexMissingException e) { assertThat(e.index().name(), equalTo("baz*")); }

it makes no sense to me that with same options, a different number of indices leads to a different result... I think we could adapt the test and remove this special case (in a separate issue it's fine too)

Ok, I'll change it via this PR.

javanna · 2015-07-09T13:26:10Z

I did a first review round, the change looks good, left a few comments.

It is not the easiest PR to review given that we are moving whole methods and slightly changing them too meanwhile. I think that the most important aspects are that convertFromWildcards gets replaced by WildcardExpressionFilter and we potentially support for more filters that will come in the future. All of the indices expansion methods are being moved from MetaData to the new IndexNameExpressionResolver dedicated class, and meanwhile we removed all of the optimizations around single index usecase etc. anything else that might have missed?

martijnvg · 2015-07-09T13:43:11Z

thanks @javanna! and those changes are correct! (nothing missed)

martijnvg · 2015-07-09T22:31:57Z

@javanna I updated the PR.

javanna · 2015-07-10T08:28:56Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

+        }
+    }
+
+    private interface IndexExpressionFilter {


I thought we said we would rename this to IndexExpressionResolver, maybe that was not clear though. what do you think about that?

javanna · 2015-07-10T08:30:18Z

left a couple more comments

martijnvg · 2015-07-10T11:58:33Z

@javanna I updated the PR.

javanna · 2015-07-10T12:03:41Z

core/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java

+                } else if (options.expandWildcardsClosed()) {
+                    indices = metaData.concreteAllClosedIndices();
+                } else {
+                    assert false : "convertFromWildcards shouldn't get called if wildcards expansion is disabled";


update the assert message given that convertFromWIldcards is not a thing anymore?

javanna · 2015-07-10T12:07:48Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java

-        assertThat(newHashSet(md.convertFromWildcards(new String[]{"testX*"}, IndicesOptions.fromOptions(true, true, true, true))), equalTo(newHashSet("testXXX", "testXXY", "testXYY")));
-        assertThat(newHashSet(md.convertFromWildcards(new String[]{"testX*"}, IndicesOptions.fromOptions(true, true, false, true))), equalTo(newHashSet("testXYY")));
-        assertThat(newHashSet(md.convertFromWildcards(new String[]{"testX*"}, IndicesOptions.fromOptions(true, true, true, false))), equalTo(newHashSet("testXXX", "testXXY")));
-    }


where did these tests go?

it is in WildcardExpressionResolverTests

javanna · 2015-07-10T12:08:52Z

left a bunch of minor comments, LGTM otherwise

…tion to IndexNameExpressionResolver. Changes in a nutshell: * All expression logic is now encapsulated by ExpressionResolver interface. * MetaData#convertFromWildcards() gets replaced by WildcardExpressionResolver. * All of the indices expansion methods are being moved from MetaData class to the new IndexNameExpressionResolver class. * All single index expansion optimisations are removed. The logic for resolving a concrete index name from an expression has been moved from MetaData to IndexExpressionResolver. The logic has been cleaned up and simplified were was possible without breaking bwc. Also the notion of aliasOrIndex has been changed to index expression. The IndexNameExpressionResolver translates index name expressions into concrete indices. The list of index name expressions are first delegated to the known ExpressionResolverS. An ExpressionResolver is responsible for translating if possible an expression into another expression (possibly but not required this can be concrete indices or aliases) otherwise the expressions are left untouched. Concretely this means converting wildcard expressions into concrete indices or aliases, but in the future other implementations could convert expressions based on different rules. To prevent many overloading of methods, DocumentRequest extends now from IndicesRequest. All implementation of DocumentRequest already did implement IndicesRequest indirectly.

This commit fixes a regression introduced with elastic#12058. This causes failures with the delete index api when providing the same index name multiple times in the request, or aliases/wildcard expressions that end up pointing to the same concrete index. The bug was revealed after merging elastic#11258 as we delete indices in batch rather than one by one. The master node will expect too many acknowledgements based on the number of indices that it's trying to delete, hence the request will never be acknowledged by all nodes. Closes elastic#14316

martijnvg added >enhancement v2.0.0-beta1 review labels Jul 6, 2015

martijnvg mentioned this pull request Jul 6, 2015

Support date math in index names for all index based apis #12059

Closed

javanna reviewed Jul 9, 2015
View reviewed changes

martijnvg mentioned this pull request Jul 9, 2015

Lookup concrete indices and alias information via a single lookup #12173

Closed

javanna reviewed Jul 10, 2015
View reviewed changes

martijnvg force-pushed the index_name_expressions branch from d41a73f to f2bc2b9 Compare July 10, 2015 11:58

javanna reviewed Jul 10, 2015
View reviewed changes

martijnvg force-pushed the index_name_expressions branch from f2bc2b9 to 52859e3 Compare July 10, 2015 13:18

martijnvg merged commit 52859e3 into elastic:master Jul 10, 2015

kevinkluge removed the review label Jul 10, 2015

javanna mentioned this pull request Oct 27, 2015

Deduplicate concrete indices after indices resolution #14316

Merged

jonaf mentioned this pull request Feb 8, 2019

Metadata construction / put-mapping requests takes too long #38654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MetaData to split off the concrete index name logic to a dedicated service #12058

Refactor MetaData to split off the concrete index name logic to a dedicated service #12058

martijnvg commented Jul 6, 2015

javanna Jul 9, 2015

martijnvg Jul 9, 2015

javanna commented Jul 9, 2015

martijnvg commented Jul 9, 2015

martijnvg commented Jul 9, 2015

javanna Jul 10, 2015

javanna commented Jul 10, 2015

martijnvg commented Jul 10, 2015

javanna Jul 10, 2015

javanna Jul 10, 2015

martijnvg Jul 10, 2015

javanna Jul 10, 2015

javanna commented Jul 10, 2015

Refactor MetaData to split off the concrete index name logic to a dedicated service #12058

Refactor MetaData to split off the concrete index name logic to a dedicated service #12058

Conversation

martijnvg commented Jul 6, 2015

javanna Jul 9, 2015

Choose a reason for hiding this comment

martijnvg Jul 9, 2015

Choose a reason for hiding this comment

javanna commented Jul 9, 2015

martijnvg commented Jul 9, 2015

martijnvg commented Jul 9, 2015

javanna Jul 10, 2015

Choose a reason for hiding this comment

javanna commented Jul 10, 2015

martijnvg commented Jul 10, 2015

javanna Jul 10, 2015

Choose a reason for hiding this comment

javanna Jul 10, 2015

Choose a reason for hiding this comment

martijnvg Jul 10, 2015

Choose a reason for hiding this comment

javanna Jul 10, 2015

Choose a reason for hiding this comment

javanna commented Jul 10, 2015