-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Avoiding watcher validation errors when a data stream points to more than one index #85507
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
Avoiding watcher validation errors when a data stream points to more than one index #85507
Conversation
Pinging @elastic/es-data-management (Team:Data Management) |
Hi @masseyke, I've created a changelog YAML for you. |
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.
Looks good! I left a question and a comments about the tests.
} | ||
|
||
if (indexAbstraction.getType() != IndexAbstraction.Type.CONCRETE_INDEX && indexAbstraction.getIndices().size() > 1) { | ||
if ((indexAbstraction.getType() != IndexAbstraction.Type.CONCRETE_INDEX |
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 if statement does look a bit complicated.
Can we instead have two simpler if statements for the valid cases?
One if statement for when indexAbstraction
is a data stream and one when indexAbstraction
is an alias.
In all other cases we would fail.
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 we also support if it's just a concrete index, right? I was not sure if there were other types I didn't know about so I left it mostly as it was. If there are definitely just those 3 though (alias, data stream, concrete index) I can definitely simplify 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.
I think we also support if it's just a concrete index, right?
I thought it was either an alias or data stream. But I might be wrong here.
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 at least one place we pass .watches
to this method, which I believe is a concrete index (although I could definitely be wrong about that).
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 changed it to explicitly look for aliases here, which does simplify things a little.
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, it does.
...plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchStoreUtilsTests.java
Outdated
Show resolved
Hide resolved
...plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchStoreUtilsTests.java
Show resolved
Hide resolved
…github.com:masseyke/elasticsearch into fix/watcher-validation-fails-with-multiple-indices
@elasticmachine update branch |
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 (pending successful build)
Failure occurred due to error message change:
java.lang.AssertionError: |
-- | --
| Expected: is "Alias [.triggered_watches] points to more than one index" |
| but: was "Alias [.triggered_watches] points to 2 indices, and does not have a designated write index" |
| at __randomizedtesting.SeedInfo.seed([8F6C099FB918BA97:988C8EAA4DCC424]:0) |
| at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18) |
| at org.junit.Assert.assertThat(Assert.java:956) |
| at org.junit.Assert.assertThat(Assert.java:923) |
| at org.elasticsearch.xpack.watcher.execution.TriggeredWatchStoreTests.testLoadingFailsWithTwoAliases(TriggeredWatchStoreTests.java:391)
(and I think bwc failures should be fixed by merging in master)
…github.com:masseyke/elasticsearch into fix/watcher-validation-fails-with-multiple-indices
@elasticmachine update branch |
…than one index (elastic#85507) This commit fixes a validation bug that prevented watcher from starting if the .watcher-history-16 data stream is pointing to more than one index. Before 8.0, .watcher-history-16 data had been an alias and had never pointed to more than one index, so this had not been a problem.
💚 Backport successful
|
…than one index (#85507) (#85631) This commit fixes a validation bug that prevented watcher from starting if the .watcher-history-16 data stream is pointing to more than one index. Before 8.0, .watcher-history-16 data had been an alias and had never pointed to more than one index, so this had not been a problem.
…than one index (elastic#85507) This commit fixes a validation bug that prevented watcher from starting if the .watcher-history-16 data stream is pointing to more than one index. Before 8.0, .watcher-history-16 data had been an alias and had never pointed to more than one index, so this had not been a problem.
…than one index (#85507) (#85755) * Avoiding watcher validation errors when a data stream points to more than one index (#85507) This commit fixes a validation bug that prevented watcher from starting if the .watcher-history-16 data stream is pointing to more than one index. Before 8.0, .watcher-history-16 data had been an alias and had never pointed to more than one index, so this had not been a problem. * Fixing test for backport
We are seeing this error in 8.0.1 if a node is restarted after the
.watcher-history-16
data stream is pointing to more than one index.This change makes it ok to use an alias with a write index or a data stream (which always has a write index).
Closes #85508