Skip to content

Conversation

@javanna
Copy link
Member

@javanna javanna commented Jul 3, 2017

This is a follow-up of #23997 and #25268.

When resolving wildcards, aliases should be treated as unavailable indices when the ignoreAliases option is set to true (currently enabled with delete index api and update aliases api). This way the allow_no_indices and ignore_unavailable options can be honoured, otherwise WildcardExpressionResolver ends up treating aliases differently and there is no way to control when an error is thrown.

The default behaviour for the delete index api, which has ignore_unavailable set to false and allow_no_indices set to true by default, is to throw an error when executed against an alias, same as when it's executed against an index that does not exist.

@javanna javanna added :Data Management/Indices APIs APIs to create and manage indices and templates >non-issue review v5.6.0 v6.0.0 labels Jul 3, 2017
@javanna javanna requested a review from dakrone July 4, 2017 07:54
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, but this LGTM. Cannot stress enough how much this needs to be rewritten and cleaned up though (in a subsequent PR), it's very difficult to reason about now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment no longer holds, since this is inside of an if statement checking that a wildcard has been seen

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, will remove

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when this is rewritten, I would prefer to have an .isConcreteIndex() method on AliasOrIndex to avoid the double negatives in comparisons, I had to stare at this for a while to validate it is working correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should either be assert false or be removed entirely, right now it's not doing anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, it's a leftover, I noticed that assertEmpty was always true so I just removed the argument. I think we can simply remove the assertion at this point.

@javanna javanna force-pushed the fix/ignore_alias_wildcard_resolver branch from c33ba00 to 4407c7b Compare July 7, 2017 13:50
javanna added 2 commits July 7, 2017 17:46
…es api

When resolving wildcards, aliases should be treated as unavailable indices when the `ignoreAliases` option is set to `true` (currently enabled with delete index api and update aliases api). This way the `allow_no_indices` and `ignore_unavailable` options can be honoured, otherwise WildcardExpressionResolver ends up treating aliases differently and there is no way to control when an error is thrown.

The default behaviour for the delete index api, which has `ignore_unavailable` set to `false` and `allow_no_indices` set to `true` by default, is to throw an error when executed against an alias, same as when it's executed against an index that does not exist.
@javanna javanna force-pushed the fix/ignore_alias_wildcard_resolver branch from 4407c7b to 594f4b9 Compare July 7, 2017 15:46
@javanna javanna removed the v5.6.0 label Jul 10, 2017
@javanna javanna merged commit a932591 into elastic:master Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Indices APIs APIs to create and manage indices and templates >non-issue v6.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants