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

Deprecated use of + in index expressions #24585

Merged
merged 5 commits into from May 15, 2017

Conversation

Projects
None yet
4 participants
@kunal642
Copy link
Contributor

commented May 10, 2017

Closes #24515

kunal642 added some commits May 10, 2017

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

@javanna Can you please have a look

@javanna
Copy link
Member

left a comment

hi @kunal642 thanks a lot for your PR, I left a couple of comments.

@@ -649,6 +652,10 @@ boolean isPreserveAliases() {
wildcardSeen = true;
}
}
if(isDeprecated) {
new DeprecationLogger(Loggers.getLogger(IndexNameExpressionResolver.class))

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

any reason why the deprecation logger is created on demand each time rather than a static field?

@@ -642,6 +642,7 @@ public void testConcreteIndicesWildcardWithNegation() {

indexNames = indexNameExpressionResolver.concreteIndexNames(state, IndicesOptions.lenientExpandOpen(), "*", "-*");
assertEquals(0, indexNames.length);
assertWarnings("use of + is deprecated in index names as it is implicit");

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

can you add a specific test method that checks the new deprecation line and remove usages of '+' elsewhere?

@@ -70,6 +71,7 @@ public void testConvertWildcardsTests() {
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+test*", "-testYYY"))), equalTo(newHashSet("testXXX", "testXYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+testX*", "+testYYY"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+testYYY", "+testX*"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
assertWarnings("use of + is deprecated in index names as it is implicit");

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

here as well, can you add a specific test method that checks the new deprecation line and remove usages of '+' elsewhere?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2017

@javanna Requested changes have been done. Please verify

@javanna
Copy link
Member

left a comment

thanks, I did another round and left a couple more comments

@@ -832,7 +831,7 @@ public void testIsPatternMatchingAllIndicesNonMatchingSingleExclusion() throws E
}

public void testIsPatternMatchingAllIndicesMatchingTrailingWildcardAndExclusion() throws Exception {
String[] indicesOrAliases = new String[]{"index*", "-index1", "+index1"};
String[] indicesOrAliases = new String[]{"index*", "-index1", "index1"};
String[] concreteIndices = new String[]{"index1", "index2", "index3"};
MetaData metaData = metaDataBuilder(concreteIndices);
assertThat(indexNameExpressionResolver.isPatternMatchingAllIndices(metaData, indicesOrAliases, concreteIndices), equalTo(true));

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

I think the tests that you deleted around '+' should be added back as a separate method. You did that for wildcard expressions but not for concrete names.

@@ -649,6 +654,9 @@ boolean isPreserveAliases() {
wildcardSeen = true;
}
}
if(isDeprecated) {

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

could you please add a whitespace after the if (before the parentheses)?

@@ -588,6 +591,7 @@ boolean isPreserveAliases() {
private Set<String> innerResolve(Context context, List<String> expressions, IndicesOptions options, MetaData metaData) {
Set<String> result = null;
boolean wildcardSeen = false;
boolean isDeprecated = false;

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

can you call it plusSeen?

@@ -649,6 +654,9 @@ boolean isPreserveAliases() {
wildcardSeen = true;
}
}
if(isDeprecated) {
DEPRECATION_LOGGER.deprecated("use of + is deprecated in index names as it is implicit");

This comment has been minimized.

Copy link
@javanna

javanna May 12, 2017

Member

change the message to "Support for '+' as part of index expressions is deprecated" ?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2017

@javanna Please review again. Changes have been made

@javanna
Copy link
Member

left a comment

I left a few very minor comments, LGTM otherwise. Thanks @kunal642

IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state, IndicesOptions.fromOptions(true, true, true, true));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+test*"))), equalTo(newHashSet("test", "test1")));
assertWarnings("support for '+' as part of index expressions is deprecated");
}

This comment has been minimized.

Copy link
@javanna

javanna May 14, 2017

Member

do you have indentation at 2 chars only for this method? We use 4 chars in our codebase. I'd appreciate if you could change that.

assertThat(newHashSet(indexNameExpressionResolver.concreteIndexNames(context, "+testX*")),
equalTo(newHashSet("testXXX", "testXXY")));
assertWarnings("support for '+' as part of index expressions is deprecated");
}

This comment has been minimized.

Copy link
@javanna

javanna May 14, 2017

Member

can you fix the indentation for this curly bracket?

@@ -128,5 +128,17 @@ public void testAll() {
private IndexMetaData.Builder indexBuilder(String index) {
return IndexMetaData.builder(index).settings(settings(Version.CURRENT).put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0));
}

public void testForDeprecatedWildCard() {

This comment has been minimized.

Copy link
@javanna

javanna May 14, 2017

Member

it is not the wildcard that's deprecated, rather the '+' . Could you rename this method accordingly please?

IndexNameExpressionResolver.WildcardExpressionResolver resolver = new IndexNameExpressionResolver.WildcardExpressionResolver();

IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state, IndicesOptions.fromOptions(true, true, true, true));
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("+test*"))), equalTo(newHashSet("test", "test1")));

This comment has been minimized.

Copy link
@javanna

javanna May 14, 2017

Member

nit: can you also add here the same few tests that you had above, which combined the use of '+' with other names in the same expression

@@ -970,4 +969,17 @@ public void testIndexAliases() {
Arrays.sort(strings);
assertArrayEquals(new String[] {"test-alias-0", "test-alias-1", "test-alias-non-filtering"}, strings);
}

public void testConcreteIndicesForDeprecatedPattern() {

This comment has been minimized.

Copy link
@javanna

javanna May 14, 2017

Member

could you slightly expand this test by adding the different combinations that we used to have above? This is just to make sure that everything still works properly.

@javanna

This comment has been minimized.

Copy link
Member

commented May 14, 2017

jenkins test this please

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@javanna Done. Please review

@javanna
Copy link
Member

left a comment

LGTM thanks @kunal642 I am going to merge this.

@javanna javanna merged commit fdb6cd8 into elastic:master May 15, 2017

1 check passed

CLA Commit author has signed the CLA
Details
@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@javanna Great, Thanks for your help

@javanna

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@kunal642 do you want to send a new PR for removing this in master?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@javanna removing the support for + in index names??

@javanna

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@kunal642 yep.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@javanna yeah sure
but why didn't we remove it all together in the master

@javanna

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@kunal642 because we tend to deprecate things first and your PR was against master in the first place. I am currently backporting the change to 5.x branch. We can now either leave it deprecated in master for a while and remove it later, or already remove it (in master only).

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@javanna okay
ill make another PR for removing this

javanna added a commit that referenced this pull request May 15, 2017

Deprecate use of + in index names (#24585)
Use of '+' in index names is implicit. There is no need to support it. This commit deprecates support for it.

Closes #24515

@clintongormley clintongormley changed the title Deprecated use of + in index names Deprecated use of + in index expressions May 15, 2017

@elasticmachine

This comment has been minimized.

Copy link

commented May 15, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@javanna After removing support for +, the index names with +test should not return any index name
right??

@javanna

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@kunal642 right, as index names cannot start with '+'. As part of index resolution, '+' prefixes should just stay untouched. Then depending on the indices option (whether ignore_unavailable is set to true or false), we throw exception or ignore the fact that such an index didn't exist.

@kunal642 kunal642 deleted the kunal642:issue#24515 branch May 15, 2017

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

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.