Remove expansion of empty index arguments in RoutingTable #10148

cbuescher · 2015-03-18T23:44:16Z

RoutingTables activePrimaryShardsGrouped(), allActiveShardsGrouped() and
allAssignedShardsGrouped() methods treated empty index array input
parameters as meaning "all" indices and expanded to the routing maps
keyset. However, the expansion of index names is now already done in
MetaData#concreteIndices(). Returning an empty index name list here
when a wildcard pattern didn't match any index name could lead to
problems like #9081 because the RoutingTable still expanded this
list of names to "_all". In case of e.g. the recovery endpoint this
could lead to problems.

This fix removes the index name expansion in RoutingTable and introduces
some more checks for preventing NPEs in MetaData#concreteIndices().

Closes #9081

RoutingTables activePrimaryShardsGrouped(), allActiveShardsGrouped() and allAssignedShardsGrouped() methods treated empty index array input parameters as meaning "all" indices and expanded to the routing maps keyset. However, the expansion of index names is now already done in MetaData#concreteIndices(). Returning an empty index name list here when a wildcard pattern didn't match any index name could lead to problems like elastic#9081 because the RoutingTable still expanded this list of names to "_all". In case of e.g. the recovery endpoint this could lead to problems. This fix removes the index name expansion in RoutingTable and introduces some more checks for preventing NPEs in MetaData#concreteIndices(). Closes elastic#9081

javanna · 2015-03-19T09:39:38Z

rest-api-spec/test/indices.recovery/10_basic.yaml

@@ -1,9 +1,6 @@
 ---
 "Indices recovery test":

-  - skip:
-      features: gtelte
-


if you rebase this change should go away right?

Yes, I deleted that on master already. Should also maybe delete the gtelte on 1.x branch?

yes I think so, should be done on 1.x too

javanna · 2015-03-19T10:10:14Z

I like the change! Left a bunch of comments

cbuescher · 2015-03-19T16:32:41Z

I adressed your comments, will revert the changes in IndicesRequestTests and create rest tests for that later. Let me know if my other replies make sense.

cbuescher · 2015-03-19T20:03:35Z

@javanna pushed a couple of changes regarding your comments. For one, I reverted the changes to IndicesRequestTests and introduced rest tests for these cases. Other changes address your comments to code style mostly. Let me know what you think.

javanna · 2015-03-20T08:28:48Z

src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java

+     * test resolving _all pattern (null, empty arry or "_all") for random IndicesOptions
+     */
+    @Test
+    public void concreteIndicesAllpatternRandom() {


concreteIndicesAllpatternRandom=>concreteIndicesAllPatternRandom

javanna · 2015-03-20T08:55:40Z

Hi @cbuescher I did another review round. it's close.

I am not sure if we should rely solely on REST tests to test the RoutingTable changes. REST tests are great but I would love to have java unit tests around them. The tests that you previously added to IndicesRequestTests were not bad, just in the wrong place. Also, I wondered if we could move them to a more isolated unit tests instead of having to send requests and read responses to find out how many shards were touched. Replacing with REST tests makes tests even less isolated, makes sense?

cbuescher · 2015-03-20T21:57:48Z

@javanna I adressed two of your comments and started adding a RoutingTable unit test, but it's still a very rudimentary test. One thing I'm currently not sure about is how to set up the RoutingTable for the test so that the shard routings that are created are in a different state than "unassigned". When I figured that out I will extend the test. Any suggestion on how to do that in a simple unit test way are.

javanna · 2015-03-23T17:56:03Z

src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java

+                assertThat(concreteIndices, notNullValue());
+                int expectedNumberOfIndices = 0;
+                if (indicesOptions.expandWildcardsOpen())
+                    expectedNumberOfIndices += 2;


brackets please

javanna · 2015-03-23T17:58:26Z

src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java

+     */
+    @Test
+    public void concreteIndicesWildcardEmptyRandom() {
+        for (int i = 0; i < 100; i++) {


wondering if we really need to repeat this 100 times. Seems a little too high, wouldn't 10 be enough? Or even just rely on the fact that tests run continously. An alternative would be to use the @Repeat annotation instead of the loop. What do you think?

Sure, will decrease that

javanna · 2015-03-23T18:06:01Z

Thanks for the update @cbuescher I had another look, looks good. You are on the right track with the RoutingTable test, looks great.

cbuescher · 2015-03-26T18:17:20Z

Added changes according to latest review comments and extended RoutingTableTest so it covers moving through different ShardRoutingStates now.

javanna · 2015-03-30T09:54:06Z

src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java

    }

    @Test
    public void testIsExplicitAllIndices_wildcard() throws Exception {
-        MetaData metaData = MetaData.builder().build();
-        assertThat(metaData.isExplicitAllPattern(new String[]{"*"}), equalTo(false));
+        assertThat(MetaData.isExplicitAllPattern(new String[]{"*"}), equalTo(false));


if you get merge conflicts on these MetaData calls that should have been static, don't worry, I think I recently merged a PR from a contributor that fixed just that.

javanna · 2015-03-30T10:02:01Z

Had another look, left a bunch of super minor comments, this is very close.

… exception message

cbuescher · 2015-03-30T11:45:45Z

I left the two exception types in MetaData.concreteIndices() but extended the test to check for throwing the correcty type. Can also make both IndexMissingExceptions, but that one wants the name of the missing index and it realy is more like a call to the function with mismatch in arguments. I hope its okay to give the IndicesOptions a simple toString() to see whats going on in case of an exception.

javanna · 2015-03-31T10:05:10Z

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

+
+        if (aliasesOrIndices == null || aliasesOrIndices.length == 0) {
+            if (!indicesOptions.allowNoIndices()) {
+                throw new ElasticsearchIllegalArgumentException("Null or zero length argument for list of index or alias names "


we throw IndexMissingException all the time when allowNoIndices is set to true. I don't think we should act differently here.

but now that I read your comment below, I see why it makes sense. Fine with illegal argument, I would only change the message to something like "no indices were specified but wildcard expansion is disabled" , shorter and more user oriented, what do you think?

cbuescher · 2015-03-31T10:56:13Z

We agreed on pulling the changes in MetaData and MetaDataTests out into a separate PR. Will open that soon. This PR now should only remove the expansion of empty or null index name lists in RoutingTable.

…Data Prevents a current edge case resolving concrete aliases or index names in cluster MetaData that could potentialy lead to NullPointerException when the IndicesOptions don't allow wildcard expansion and the method is called with aliasesOrIndices argument null or emtpy list. This change adds a check for that and introduces randomized test that catches this. Closes #10342 Closes #10339

cbuescher · 2015-04-01T10:18:34Z

@javanna With the removal of the changes in MetaData to #10339, anything else to change here?

javanna · 2015-04-01T16:04:47Z

LGTM @cbuescher thanks

RoutingTables activePrimaryShardsGrouped(), allActiveShardsGrouped() and allAssignedShardsGrouped() methods treated empty index array input parameters as meaning "all" indices and expanded to the routing maps keyset. However, the expansion of index names is now already done in MetaData#concreteIndices(). Returning an empty index name list here when a wildcard pattern didn't match any index name could lead to problems like #9081 because the RoutingTable still expanded this list of names to "_all". In case of e.g. the recovery endpoint this could lead to problems. Closes #9081 Closes #10148

cbuescher added >bug v2.0.0-beta1 :Data Management/Indices APIs APIs to create and manage indices and templates v1.6.0 review labels Mar 18, 2015

javanna reviewed Mar 19, 2015
View reviewed changes

Christoph Büscher added 2 commits March 19, 2015 11:34

Changed nesting in MetaDataTests testing for exceptions

880959c

Moved tests from IndicesRequestTests to yaml tests

501076c

javanna reviewed Mar 20, 2015
View reviewed changes

javanna self-assigned this Mar 20, 2015

Christoph Büscher added 2 commits March 20, 2015 12:36

Minor deletes and renames

6ec4787

Added RoutingTableTest

8cae7d4

javanna reviewed Mar 23, 2015
View reviewed changes

Christoph Büscher added 2 commits March 26, 2015 13:09

Include changes for comments on MetaData and tests

3a62abc

Adding to RoutingTableTest

0e5b1d1

javanna reviewed Mar 30, 2015
View reviewed changes

Christoph Büscher added 2 commits March 30, 2015 12:37

Prepending test* to all methods in MetaDataTests

aff7703

Adding detailed check for exception in MetaDataTests, more meaningful…

8833b1d

… exception message

javanna reviewed Mar 31, 2015
View reviewed changes

Reverting changes in MetaData and tests

b9edf98

cbuescher mentioned this pull request Mar 31, 2015

Checks for preventing NPEs in MetaData#concreteIndices #10339

Closed

cbuescher closed this in 2beda39 Apr 2, 2015

clintongormley removed the review label Aug 7, 2015

cbuescher deleted the fix/9081 branch March 31, 2016 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove expansion of empty index arguments in RoutingTable #10148

Remove expansion of empty index arguments in RoutingTable #10148

cbuescher commented Mar 18, 2015

javanna Mar 19, 2015

cbuescher Mar 19, 2015

javanna Mar 19, 2015

cbuescher Mar 19, 2015

javanna commented Mar 19, 2015

cbuescher commented Mar 19, 2015

cbuescher commented Mar 19, 2015

javanna Mar 20, 2015

javanna commented Mar 20, 2015

cbuescher commented Mar 20, 2015

javanna Mar 23, 2015

javanna Mar 23, 2015

cbuescher Mar 26, 2015

javanna commented Mar 23, 2015

cbuescher commented Mar 26, 2015

javanna Mar 30, 2015

javanna commented Mar 30, 2015

cbuescher commented Mar 30, 2015

javanna Mar 31, 2015

javanna Mar 31, 2015

cbuescher commented Mar 31, 2015

cbuescher commented Apr 1, 2015

javanna commented Apr 1, 2015

Remove expansion of empty index arguments in RoutingTable #10148

Remove expansion of empty index arguments in RoutingTable #10148

Conversation

cbuescher commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Mar 19, 2015

cbuescher commented Mar 19, 2015

cbuescher commented Mar 19, 2015

Choose a reason for hiding this comment

javanna commented Mar 20, 2015

cbuescher commented Mar 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Mar 23, 2015

cbuescher commented Mar 26, 2015

Choose a reason for hiding this comment

javanna commented Mar 30, 2015

cbuescher commented Mar 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Mar 31, 2015

cbuescher commented Apr 1, 2015

javanna commented Apr 1, 2015