clear auto-follow errors on deleting pattern#80544
Conversation
|
Pinging @elastic/es-distributed (Team:Distributed) |
| ); | ||
|
|
||
| // then stats are removed as well | ||
| assertThat(autoFollowCoordinator.getStats().getRecentAutoFollowErrors().keySet(), empty()); |
There was a problem hiding this comment.
This could check that unrelated errors are not removed
| ) | ||
| ); | ||
| // and applied | ||
| assertThat(autoFollowCoordinator.getStats().getRecentAutoFollowErrors(), hasKey("pattern1:index1")); |
There was a problem hiding this comment.
I was surprised that the second AutoFollowResult cleared the error from the first one.
Not sure if this is intended behavior.
henningandersen
left a comment
There was a problem hiding this comment.
I left a few smaller comments, otherwise looking good.
henningandersen
left a comment
There was a problem hiding this comment.
I left a few comments, will review the latest addition shortly.
| } | ||
| assert assertNoOtherActiveAutoFollower(newAutoFollowers); | ||
| this.autoFollowers = autoFollowers.copyAndPutAll(newAutoFollowers).copyAndRemoveAll(removedRemoteClusters); | ||
| this.patterns = Set.copyOf(autoFollowMetadata.getPatterns().keySet()); |
There was a problem hiding this comment.
I think this is too late, it should preferably go before starting any new auto-followers. Otherwise, we risk a race where the auto-follower completes and updates stats before we get here (would have to be twice to become a problem but anyway we can fix this by assigning this earlier).
| assertThat(before.getRecentAutoFollowErrors().keySet(), contains("pattern1", "pattern2", "pattern3")); | ||
| assertThat(after.getRecentAutoFollowErrors().keySet(), allOf(contains("pattern1", "pattern2"), not(contains("pattern3")))); |
There was a problem hiding this comment.
This is slightly confusing. contains verifies same length and same order, which gives rise to two comments:
- I think the order is not relevant. So
equalToaSet.ofwould be better here, alternativelycontainsInAnyOrder. The test only passes (I think) because of the use ofLinkedHashMapinternally. - The
not(contains("pattern3"))is true because it is not a list containing only that one element.
I think we should simply use equalTo instead.
I think this comment is true for the other contains verifications below in this PR.
henningandersen
left a comment
There was a problem hiding this comment.
One minor nit here, otherwise this looks good.
| } | ||
|
|
||
| this.patterns = Set.copyOf(autoFollowMetadata.getPatterns().keySet()); | ||
|
|
There was a problem hiding this comment.
I think we should set the patterns to the empty list above when autoFollowMetadata == null too (though I think we never go back to null, so mostly for completeness).
There was a problem hiding this comment.
I believe we are also not clearing/stopping autoFollowers once metadata is null. If this happens for whatever reason then it might result in auto-followers still running but not producing any stats
There was a problem hiding this comment.
Good point, perhaps we can simply add an assert before the return above to express the intent here?
# Conflicts: # x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java
(cherry picked from commit 0f10825)
(cherry picked from commit 0f10825)
This change clears auto-follow errors that belongs to patterns that are removed by cluster update.
Closes: #77723