-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Handle the existence of system data streams in Get Aliases API #73244
Handle the existence of system data streams in Get Aliases API #73244
Conversation
This commit adjusts the behavior of the Get Aliases API to more thoroughly prevent errors and warnings from being emitted unnecessarily from the Get Aliases API when system data streams exist. This is a hack which should be removed as soon as possible.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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 reviewed Jay's changes and this is a better fix all-around - it's slightly more code, but it's making the fix in the right place vs. piling on more hacks.
// system index that backs system data stream | ||
if (indexAbstraction.getParentDataStream() != null) { | ||
if (indexAbstraction.getParentDataStream().isSystem() == false) { | ||
throw new IllegalStateException("system index is part of a data stream that is not a system data stream"); |
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 this is a worth candidate for promoting to an assert
- we should never hit this case, right?
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.
it should never hit this case but I prefer this actually being detected in the wild since we don't run with assertions on normally
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 meant along the lines of:
assert false : "this means you probably wrote a bug that lets system indices be part of non-system data streams";
throw new IllegalStateException("system index is part of a data stream that is not a system data stream");
This shouldn't block the PR though, more maybe as a follow-up?
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.
LGTM, but given the change has moved in a direction outside of my immediate area, I may not be best placed to review it.
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.
LGTM - I'm glad we ended up with this fix rather than just tweaking our hack. Great work, Jay and Gordon.
…ic#73244) This commit adjusts the behavior of the Get Aliases API to more thoroughly prevent errors and warnings from being emitted unnecessarily from the Get Aliases API by retrieving all indices including system ones and only warning in the post processing of the action. Additionally, the IndexAbstractionResolver has been updated to properly handle system data streams when evaluating visibility. Closes elastic#73218 Co-authored-by: jaymode <jay@elastic.co>
This commit adjusts the behavior of the Get Aliases API to more
thoroughly prevent errors and warnings from being emitted unnecessarily
from the Get Aliases API by retrieving all indices including system ones
and only warning in the post processing of the action.
Additionally, the IndexAbstractionResolver has been updated to properly
handle system data streams when evaluating visibility.
Closes #73218
Co-authored-by: jaymode jay@elastic.co