-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Wrong behavior deleting alias #23997
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
Conversation
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? |
This looks correct to me but I am not very familiar with aliases. |
@@ -125,6 +126,47 @@ public void testAll() { | |||
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("_all"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY"))); | |||
} | |||
|
|||
// issue #23960 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I would prefer a straightforward comment explaining the issue being tested. A link to a GitHub issues means that I have to be online and context switch to obtain more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasontedor Good point! I just followed the lead of another test in this class which is abbreviated only with the issue number.
May I assume that generally a test should be commented with a short description why the test has been introduced (and maybe the issue for further reference)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test and its purpose is straightforward, I would say that no comment is necessary. If the test is for an tricky bug, or its purpose is unclear, I would say a comment is a necessity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasontedor Thank you for answering a trivial question! I understand this is common sense, but somehow common sense manages to differ between projects... Do you know if there is a resource where the basic coding practices are defined for new comers? If there is, a link will be really appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of what you're looking for is not written down, it's tribal knowledge that we all accumulate with experience in the project. We explain some straightforward things in the contributing docs but the vast majority of our conventions are not written down (and I think that's okay, we catch them during code review).
@@ -643,6 +643,33 @@ public void testConcreteIndicesWildcardWithNegation() { | |||
assertEquals(0, indexNames.length); | |||
} | |||
|
|||
// issue #23960 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I would prefer a straightforward comment explaining the issue being tested. A link to a GitHub issues means that I have to be online and context switch to obtain more information.
Also these things change with time. Now we prefer comments explaining the
test without a link because that is easier to read in line and if we need
the issue we can blame it. But this hasn't always been the norm.
Still, it'd be good to stick it in contributing.md.
…On Thu, Apr 13, 2017, 11:31 PM Jason Tedor ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
core/src/test/java/org/elasticsearch/cluster/metadata/WildcardExpressionResolverTests.java
<#23997 (comment)>
:
> @@ -125,6 +126,47 @@ public void testAll() {
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("_all"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY")));
}
+ // issue #23960
I think a lot of what you're looking for is not written down, it's tribal
knowledge that we all accumulate with experience in the project. We explain
some straightforward things in the contributing docs but the vast majority
of our conventions are not written down (and I think that's okay, we catch
them during code review).
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#23997 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANLorYZAGCQGKJ7wkRjQCqpf_LQUv5Yks5rvuiCgaJpZM4M3-7u>
.
|
The index wildcard is usually resolved against all indices and aliases in the cluster. Only for the index alias action the index wildcard will be resolved only against the indices. I could have introduced a new |
hi @olcbean thanks for the PR! I think the same fix would be needed in get alias api ( |
To be clear, I would make it an internal only new index option, that is not settable through REST, similar to |
f56312d
to
d44fc9a
Compare
@javanna thank you for your feedback and putting this into prospective :) However I am not really sure that with this change the #2318 can be solved directly. According to a later comment in the very same thread #2318 (comment)
but with the new option all wildcards will be resolved only to indices, so no exception can possibly be thrown. Do you think that this is an acceptable deviation from the described behavior? |
Yes, the reason why we went that route is that I was against introducing a new index option at the time, but given that we need in it in more than in one place now, I changed my mind :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR @olcbean . I reviewed it and left some comments
public boolean ignoreAliases() { | ||
return (id & IGNORE_ALIASES) != 0; | ||
} | ||
|
||
public void writeIndicesOptions(StreamOutput out) throws IOException { | ||
out.write(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method and the read need to handle backwards compatibility: see 7548b2e for a similar change with bw comp layer from when we added allowAliasesToMultipleIndices
@@ -61,7 +61,7 @@ | |||
|
|||
//indices options that require every specified index to exist, expand wildcards only to open indices and | |||
//don't allow that no indices are resolved from wildcard expressions | |||
private static final IndicesOptions INDICES_OPTIONS = IndicesOptions.fromOptions(false, false, true, false); | |||
private static final IndicesOptions INDICES_OPTIONS = IndicesOptions.fromOptions(false, false, true, false, true, false, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we have to change also the default value for GetAliasesRequest ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.. I was considering to leave the changes to GetAliasesRequest
for another PR ( for better traceability ). But np, I will include them in this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine either way, if you prefer you can send another followup PR to address that.
if (context.getOptions().ignoreAliases()) { | ||
return subMap.entrySet().stream() | ||
.filter(p -> !p.getValue().isAlias()) | ||
.collect(Collectors.toMap(p -> p.getKey(), p -> p.getValue())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: replace the lambda expressions with method references? Map.Entry::getKey
and Map.Entry::getValue
?
final String pattern = expression; | ||
return metaData.getAliasAndIndexLookup() | ||
.entrySet() | ||
.stream() | ||
.filter(e -> context.getOptions().ignoreAliases() ? !e.getValue().isAlias() : true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.filter(e -> context.getOptions().ignoreAliases() == false || e.getValue().isAlias() == false)
?
@@ -625,7 +626,7 @@ boolean isPreserveAliases() { | |||
} | |||
|
|||
final IndexMetaData.State excludeState = excludeState(options); | |||
final Map<String, AliasOrIndex> matches = matches(metaData, expression); | |||
final Map<String, AliasOrIndex> matches = matches(context, metaData, expression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we are fixing wildcard matching, but shouldn't we also honour the new option when resolving provided concrete names? e.g. what if I specify an alias amongst the indices, that are supposed to be concrete indices names only? I think if we fix this, it would also help with #10106 as we would not allow to create an alias that points to another alias anymore (which does not do what users think it does).
@@ -643,6 +644,51 @@ public void testConcreteIndicesWildcardWithNegation() { | |||
assertEquals(0, indexNames.length); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add tests for the new option when resolving expressions that don't contain wildcards?
@@ -643,6 +644,51 @@ public void testConcreteIndicesWildcardWithNegation() { | |||
assertEquals(0, indexNames.length); | |||
} | |||
|
|||
// concreteIndexNames must be able to resolve the provided wildcard only against the defined | |||
// indices when ignoreAliases option is set or against the defined indices and aliases otherwise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to think that this comment is not needed. Maybe add a small comment close to where you create the two indices options?
@@ -125,6 +126,50 @@ public void testAll() { | |||
assertThat(newHashSet(resolver.resolve(context, Arrays.asList("_all"))), equalTo(newHashSet("testXXX", "testXYY", "testYYY"))); | |||
} | |||
|
|||
// WildcardExpressionResolver must be able to resolve the provided wildcard only against the | |||
// defined indices when ignoreAliases option is set or against the defined indices and aliases | |||
// otherwise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
equalTo(newHashSet("foo_foo"))); | ||
|
||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding a couple of integration tests to IndexAliasesIT
also? Maybe test both IndicesAliasesRequest and GetAliasesRequest ?
@javanna thank you for the review! I hope the latest commit addresses the raised concerns ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @olcbean thanks a lot for updating, I did another review round, it looks very good, I left some more comments but nothing major, it's getting close!
@@ -120,12 +121,16 @@ public boolean ignoreAliases() { | |||
} | |||
|
|||
public void writeIndicesOptions(StreamOutput out) throws IOException { | |||
out.write(id); | |||
if (out.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this is still mark unreleased, we have already released alpha1, I think you should use alpha2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to rebase to see alpha2 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh good point, maybe leave that as the last change then so it doesn't mess up the diffs for review :)
if (out.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { | ||
out.write(id); | ||
} else { | ||
out.write(id & 0x3f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment on what this does and why for future reference?
public void writeIndicesOptions(StreamOutput out) throws IOException { | ||
out.write(id); | ||
if (out.getVersion().onOrAfter(Version.V_6_0_0_alpha1_UNRELEASED)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you expand IndicesOptionsTests#testSerialization to pass in also the ignoreAliases flag when creating the random indices options and check that they get written/read correctly. You will need an if based on the version there as well.
forbidClosedIndices, false); | ||
} | ||
|
||
public static IndicesOptions fromOptions(boolean ignoreUnavailable, boolean allowNoIndices, boolean expandToOpenIndices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you expand IndicesOptionsTests#testFromOptions using and testing this new flag too?
SortedMap<String,AliasOrIndex> subMap = metaData.getAliasAndIndexLookup().subMap(fromPrefix, toPrefix); | ||
if (context.getOptions().ignoreAliases()) { | ||
return subMap.entrySet().stream() | ||
.filter(p -> !p.getValue().isAlias()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do here as well p.getValue().isAlias() == false
?
assertTrue(indexNames.contains("bar_bar")); | ||
|
||
indexNames = Arrays.asList(indexNameExpressionResolver.concreteIndexNames(state, indicesAndAliasesOptions, "foo")); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the empty lines between assigning indexNames the corresponding assertions? Also above? If you want to divide this into blocks you could also do:
{
List<String> indexNames = Arrays.asList(indexNameExpressionResolver.concreteIndexNames(state, indicesAndAliasesOptions, "foo*"));
assert.....
}
This way you declare the list each time and it doesn't get reused.
// when ignoreAliases option is not set, WildcardExpressionResolver resolves the provided | ||
// expressions against the defined indices and aliases | ||
IndicesOptions indicesAndAliasesOptions = IndicesOptions.fromOptions(false, false, true, false, true, false, false); | ||
IndexNameExpressionResolver.Context context = new IndexNameExpressionResolver.Context(state, indicesAndAliasesOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you rename this to highlight what it holds? contextIndicesAndAliases?
// ignoreAliases option is set, WildcardExpressionResolver resolves the provided expressions | ||
// only against the defined indices | ||
IndicesOptions ignoreAliasesOptions = IndicesOptions.fromOptions(false, false, true, false, true, false, true); | ||
IndexNameExpressionResolver.Context contextIndices = new IndexNameExpressionResolver.Context(state, ignoreAliasesOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contextIndicesOnly ?
@@ -177,6 +178,9 @@ public IndexNameExpressionResolver(Settings settings) { | |||
final Set<Index> concreteIndices = new HashSet<>(expressions.size()); | |||
for (String expression : expressions) { | |||
AliasOrIndex aliasOrIndex = metaData.getAliasAndIndexLookup().get(expression); | |||
if (context.getOptions().ignoreAliases() && aliasOrIndex.isAlias()) { | |||
aliasOrIndex = null; | |||
} | |||
if (aliasOrIndex == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change change this to if (aliasOrIndex == null || (aliasOrIndex.isAlias() && context.getOptions().ignoreAliases()))
and remove the new if above
assertThat(admin().indices().prepareAliasesExist("foo").setIndices("foo_foo").get().exists(), equalTo(false)); | ||
assertThat(admin().indices().prepareAliasesExist("foo").setIndices("bar_bar").get().exists(), equalTo(true)); | ||
expectThrows(IndexNotFoundException.class, () -> admin().indices().prepareAliases() | ||
.addAliasAction(AliasActions.remove().index("foo").alias("foo")).execute().actionGet()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add a test that makes sure that when adding an alias, you can not make it point to another alias anymore as the indices get resolved to indices only?
@javanna thank you for your remarks! I will integrate them later in the week. Just one more question : do I need to modify some documentation as well ( maybe indices.asciidoc )? |
very good point @olcbean . A note on the migrate guide would be nice, although we are fixing a bug. We are introducing a change in the behaviour of get aliases and indices aliases api so better be on the safe side and list it there. |
@javanna, I just pushed some more changes. Hopefully the last round ;) |
Thanks a lot @olcbean for the great work, I just merged this. |
* master: Add support for clear scroll to high level REST client (elastic#25038) Tiny correction in inner-hits.asciidoc (elastic#25066) Added release notes for 6.0.0-alpha2 Expand index expressions against indices only when managing aliases (elastic#23997) Collapse inner hits rest test should not skip 5.x Settings: Fix secure settings by prefix (elastic#25064) add `exclude_keys` option to KeyValueProcessor (elastic#24876) Test: update missing body tests to run against versions >= 5.5.0 Track EWMA[1] of task execution time in search threadpool executor Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current). Fixed NPEs caused by requests without content. (elastic#23497) Plugins can register pre-configured char filters (elastic#25000) Build: Allow preserving shared dir (elastic#24962) Tests: Make secure settings available from settings builder for tests (elastic#25037)
* master: (1210 commits) Add support for clear scroll to high level REST client (elastic#25038) Tiny correction in inner-hits.asciidoc (elastic#25066) Added release notes for 6.0.0-alpha2 Expand index expressions against indices only when managing aliases (elastic#23997) Collapse inner hits rest test should not skip 5.x Settings: Fix secure settings by prefix (elastic#25064) add `exclude_keys` option to KeyValueProcessor (elastic#24876) Test: update missing body tests to run against versions >= 5.5.0 Track EWMA[1] of task execution time in search threadpool executor Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current). Fixed NPEs caused by requests without content. (elastic#23497) Plugins can register pre-configured char filters (elastic#25000) Build: Allow preserving shared dir (elastic#24962) Tests: Make secure settings available from settings builder for tests (elastic#25037) [TEST] Skip wildcard expansion test due to breaking change Test that gradle and Java version types match (elastic#24943) Include duplicate jar when jarhell check fails Change ScriptContexts to use needs instead of uses$. (elastic#25036) Change `has_child`, `has_parent` queries and `childen` aggregation to work with the new join field type and at the same time maintaining support for the `_parent` meta field type. Remove comma-separated feature parsing for GetIndicesAction ...
* master: (619 commits) Add support for clear scroll to high level REST client (elastic#25038) Tiny correction in inner-hits.asciidoc (elastic#25066) Added release notes for 6.0.0-alpha2 Expand index expressions against indices only when managing aliases (elastic#23997) Collapse inner hits rest test should not skip 5.x Settings: Fix secure settings by prefix (elastic#25064) add `exclude_keys` option to KeyValueProcessor (elastic#24876) Test: update missing body tests to run against versions >= 5.5.0 Track EWMA[1] of task execution time in search threadpool executor Removes an invalid assert in resizing big arrays which does not always hold (resizing can result in a smaller size than the current size, while the assert attempted to verify the new size is always greater than the current). Fixed NPEs caused by requests without content. (elastic#23497) Plugins can register pre-configured char filters (elastic#25000) Build: Allow preserving shared dir (elastic#24962) Tests: Make secure settings available from settings builder for tests (elastic#25037) [TEST] Skip wildcard expansion test due to breaking change Test that gradle and Java version types match (elastic#24943) Include duplicate jar when jarhell check fails Change ScriptContexts to use needs instead of uses$. (elastic#25036) Change `has_child`, `has_parent` queries and `childen` aggregation to work with the new join field type and at the same time maintaining support for the `_parent` meta field type. Remove comma-separated feature parsing for GetIndicesAction ...
With elastic#23997 we have introduced a new internal index option that allows to resolve index expressions only against concrete indices while ignoring aliases. Such index option was applied to IndicesAliasesRequest, so that the index part of alias actions would only be resolved against concrete indices. Same is done in this commit with delete index request. Deleting aliases has always been confusing as some users expect it to only remove the alias from the index (which has its own specific API). Even worse, in case of filtered aliases, deleting an alias may leave users with the expectation that only the documents that match the filter are deleted, which was never the case. To address all this confusion, delete index api works now only against concrete indices. WIldcard expressions will be only resolved against concrete index, as if aliases didn't exist. If one tries to delete against an alias, an IndexNotFoundException will be thrown regardless of whether the alias exists or not, as a concrete index with such a name doesn't exist. Closes elastic#2318
With #23997 we have introduced a new internal index option that allows to resolve index expressions only against concrete indices while ignoring aliases. Such index option was applied to IndicesAliasesRequest, so that the index part of alias actions would only be resolved against concrete indices. Same is done in this commit with delete index request. Deleting aliases has always been confusing as some users expect it to only remove the alias from the index (which has its own specific API). Even worse, in case of filtered aliases, deleting an alias may leave users with the expectation that only the documents that match the filter are deleted, which was never the case. To address all this confusion, delete index api works now only against concrete indices. WIldcard expressions will be only resolved against concrete index, as if aliases didn't exist. If one tries to delete against an alias, an IndexNotFoundException will be thrown regardless of whether the alias exists or not, as a concrete index with such a name doesn't exist. Closes #2318
Support for providing aliases in the index side of update aliases, put alias and delete alias API will be removed in 6.0. This commit adds a deprecation warning for update aliases, put alias or delete alias request that specifies aliases, or wildcard expressions that match aliases, on their index side. Relates to elastic#23997
With elastic#23997 and elastic#25268 we have changed put alias, delete alias, update aliases and delete index to not accept aliases. Instead concrete indices should be provided as their index parameter. This commit improves the error message in case aliases are provided, from an IndexNotFoundException (404 status code) with "no such index" message, to an IllegalArgumentException (400 status code) with "The provided expression [alias] matches an alias, specify the corresponding concrete indices instead." message. Note that there is no specific error message for the case where wildcard expressions match one or more aliases. In fact, aliases are simply ignored when expanding wildcards for such APIs. An error is thrown only when the expression ends up matching no indices at all, and allow_no_indices is set to false. In that case the error is still the generic "404 - no such index".
With #23997 and #25268 we have changed put alias, delete alias, update aliases and delete index to not accept aliases. Instead concrete indices should be provided as their index parameter. This commit improves the error message in case aliases are provided, from an IndexNotFoundException (404 status code) with "no such index" message, to an IllegalArgumentException (400 status code) with "The provided expression [alias] matches an alias, specify the corresponding concrete indices instead." message. Note that there is no specific error message for the case where wildcard expressions match one or more aliases. In fact, aliases are simply ignored when expanding wildcards for such APIs. An error is thrown only when the expression ends up matching no indices at all, and allow_no_indices is set to false. In that case the error is still the generic "404 - no such index".
Indices wildcards were resolved against all indices and aliases. And if an alias matched, then all indices with this alias were returned as matching.
In other words
will actually return both indices
foo_foo
andbar_bar
as the common aliasfoo
matched the wildcard. Which led to deleting the alias for both indices.Related to #23960
Relates to #10106