MetaData Builder doesn't properly prevent an alias with the same name as an index#26804
Merged
bleskes merged 2 commits intoelastic:masterfrom Sep 29, 2017
Merged
MetaData Builder doesn't properly prevent an alias with the same name as an index#26804bleskes merged 2 commits intoelastic:masterfrom
bleskes merged 2 commits intoelastic:masterfrom
Conversation
… of index Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it.
dakrone
requested changes
Sep 27, 2017
Member
dakrone
left a comment
There was a problem hiding this comment.
Thanks for fixing this! I left some comments
| } | ||
| assert duplicates.size() > 0; | ||
| throw new IllegalStateException("index and alias names need to be unique, but the following duplicates were found [" | ||
| + Strings.join(duplicates, ", ")+ "]"); |
Member
There was a problem hiding this comment.
I think this should be Strings.collectionToCommaDelimitedString and then you can use our Strings.java instead of JOpt's version
|
|
||
| List<String> allOpenIndicesLst = new ArrayList<>(); | ||
| List<String> allClosedIndicesLst = new ArrayList<>(); | ||
| final Set<String> allIndices = new HashSet<>(); |
Member
There was a problem hiding this comment.
Minor nit, this can be new HashSet<>(indices.size())
| } | ||
| } | ||
| } | ||
| assert duplicates.size() > 0; |
Member
There was a problem hiding this comment.
Perhaps add the duplicate size to the assert message here
Member
There was a problem hiding this comment.
whoops I read it backwards, so yeah, not really necessary
| final Set<String> allIndices = new HashSet<>(); | ||
| final List<String> allOpenIndices = new ArrayList<>(); | ||
| final List<String> allClosedIndices = new ArrayList<>(); | ||
| final Set<String> duplicateAliasesIndices = new HashSet<>(); |
Member
There was a problem hiding this comment.
This can just be named allAliasNames since it's just a set of the alias names, the "duplicate" part was throwing me off
Contributor
Author
There was a problem hiding this comment.
I started that way, but since Java doesn't have a normal non set modifying set difference I had to mutate this set. I felt it's dangerous to just call it allAliases and then essentially make it empty.
| for (ObjectCursor<IndexMetaData> cursor : indices.values()) { | ||
| IndexMetaData indexMetaData = cursor.value; | ||
| aliasAndIndexLookup.put(indexMetaData.getIndex().getName(), new AliasOrIndex.Index(indexMetaData)); | ||
| AliasOrIndex exiting = aliasAndIndexLookup.put(indexMetaData.getIndex().getName(), new AliasOrIndex.Index(indexMetaData)); |
| } else { | ||
| throw new IllegalStateException("unexpected alias [" + aliasMetaData.getAlias() + "][" + aliasOrIndex + "]"); | ||
| assert alias instanceof AliasOrIndex.Alias : alias.getClass().getName(); | ||
| ((AliasOrIndex.Alias) alias).addIndex(indexMetaData); |
Member
There was a problem hiding this comment.
I think this would be cleaner as
aliasAndIndexLookup.compute(aliasMetaData.getAlias(), (aliasName, alias) -> {
if (alias == null) {
return new AliasOrIndex.Alias(aliasMetaData, indexMetaData);
} else {
((AliasOrIndex.Alias) alias).addIndex(indexMetaData);
return alias;
}
});
Contributor
Author
|
thx @dakrone |
bleskes
added a commit
that referenced
this pull request
Sep 29, 2017
… as an index (#26804) Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it. Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.
bleskes
added a commit
that referenced
this pull request
Sep 29, 2017
… as an index (#26804) Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it. Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.
bleskes
added a commit
that referenced
this pull request
Sep 29, 2017
… as an index (#26804) Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the `MetaData.Builder#build()` method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index). This commit fixes the above and improves the error message while at it. Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 1, 2017
* xdcr: Maybe die before trying to log cause Log cause when a write and flush fails Die if write listener fails due to fatal error RecoveryIT.testHistoryUUIDIsGenerated should reduce unassigned shards delay instead of ensure green. Replace group map settings with affix setting (elastic#26819) Fix references to vm.max_map_count in Docker docs Add more instructions about jar hell (elastic#26837) Forbid negative values for index.unassigned.node_left.delayed_timeout (elastic#26828) Nitpicking typos in comments (elastic#26831) MetaData Builder doesn't properly prevent an alias with the same name as an index (elastic#26804)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Elasticsearch doesn't allow having an index alias named with the same name as an existing index. We currently have logic that tries to prevents that in the
MetaData.Builder#build()method. Sadly that logic is flawed. Depending on iteration order, we may allow the above to happen (if we encounter the alias before the index).This commit fixes the above and improves the error message while at it.
Note that we have a lot of protections in place before we end up relying on the metadata builder (validating this when we process APIs). I takes quite an abuse of the cluster to get that far.