#111433 Watch Next Run Interval Resets On Shard Move or Node Restart #115102
+256
−2
Conversation
This file contains 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
when restarting, moving shards or resuming from stopped.
lukewhiting
added
>bug
:Data Management/Watcher
auto-backport
|
Hi @lukewhiting, I've created a changelog YAML for you. |
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
masseyke
reviewed
Oct 21, 2024
| fail("waiting too long for all watches to be triggered"); | ||
| } | ||
|
|
||
| advanceClockIfNeeded(clock.instant().plusMillis(1100).atZone(ZoneOffset.UTC)); |
There was a problem hiding this comment.
It would be good to have short comments on each of these 4 tests describing what they're for. It took me a few minutes to figure out how they all differed. And in the case of this test, are you trying to show that it does not execute too many times if you advance the clock a good bit? Is it worth adding another latch or two to prove that the watch hasn't run when you expect it not to have run yet?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes #111433 by making initial scheduling of IntervalSchedules within the watcher
TickerScheduleTriggerEngineaware of the correct last run time of a watch, rather than using the current time. This allows it to correctly calculate the next runtime rather than waiting the full duration of the watch's interval before starting the first run.This code uses the last time the watch started execution (
lastCheckedTime) by default, falling back first to the last time the watch was last activated (which is relevant for newly created or recently unpaused tasks) before finally falling back tonow()as a last resort.The use of the last start time vs last completion time (which is not persistently stored at the moment) does mean we don't need to store a new item for every watch in the cluster state, reducing bloat, but it does mean that a watch that is migrated to a new node mid run may execute early on it's next run. I think this tradeoff is acceptable given the rarity and low impact of such a scenario VS the impact of increasing the cluster state size for every watch added.