Watcher: Only trigger a watch if new or schedule/changed #35908
Merged
Conversation
This file contains hidden or 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
The trigger engine did always create a new schedule data structure, when the watcher indexing listener called an add. However the indexing listener also called add, when the watch status was updated. This means, that upon a watch status update the watch got retriggered, potentially waiting a defined interval from the watch status update onwards, instead of waiting from the last run. This commit only updates the schedule in the trigger engine, if it actually has changed, otherwise the existing schedule will not be touched. This has two results 1. If a watch is updated by an execution, the existing interval will not be touched (meaning the scheduled time will not move forward). 2. If a watch is updated by a user, but the schedule is not changed, it will not be reset from the update (for example starting to count from 5 minutes again, if the interval was set to 5 minutes). Furthermore some minor cleanups were applied, making variables final in the ctor, preventing double creation of variables.
Collaborator
|
Pinging @elastic/es-core-features |
hub-cap
approved these changes
Nov 26, 2018
jakelandis
approved these changes
Nov 27, 2018
Contributor
jakelandis
left a comment
jakelandis
left a comment
There was a problem hiding this comment.
A couple minor/questions but LGTM as-is too.
| // resulting in later executions, as the time would only count after a watch has been stored, as this code is triggered by the | ||
| // watcher indexing listener | ||
| // this also means that updating an existing watch would not retrigger the schedule time, if it remains the same schedule | ||
| if (currentSchedule == null || currentSchedule.schedule.equals(trigger.getSchedule()) == false) { |
Contributor
There was a problem hiding this comment.
minor: should we add a comment to the Schedule interface that implementations must implement .equals ? (else this check may silently fail)
Contributor
Author
There was a problem hiding this comment.
indeed. the only way to really fix that would be to implement sth like Comparable, but that is not really needed from a functional perspective. I have added a comment for now.
| new DateTime(triggeredTime, UTC), new DateTime(scheduledTime, UTC)); | ||
| events.add(new ScheduleTriggerEvent(schedule.name, new DateTime(triggeredTime, UTC), | ||
| new DateTime(scheduledTime, UTC))); | ||
| DateTime triggeredDateTime = new DateTime(triggeredTime, UTC); |
Contributor
There was a problem hiding this comment.
question: curious why not Java time here ?
Contributor
Author
There was a problem hiding this comment.
the datetime is used two lines below in the CTOR. We will work on switching fully to java time in the future.
| public void accept(Iterable<TriggerEvent> events) { | ||
| events.forEach(triggerEvents::add); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
minor: Is the register and triggerEvents needed here ?
spinscale
added a commit
that referenced
this pull request
Nov 27, 2018
The trigger engine did always create a new schedule data structure, when the watcher indexing listener called an add. However the indexing listener also called add, when the watch status was updated. This means, that upon a watch status update the watch got retriggered, potentially waiting a defined interval from the watch status update onwards, instead of waiting from the last run. This commit only updates the schedule in the trigger engine, if it actually has changed, otherwise the existing schedule will not be touched. This has two results 1. If a watch is updated by an execution, the existing interval will not be touched (meaning the scheduled time will not move forward). 2. If a watch is updated by a user, but the schedule is not changed, it will not be reset from the update (for example starting to count from 5 minutes again, if the interval was set to 5 minutes). Furthermore some minor cleanups were applied, making variables final in the ctor, preventing double creation of variables.
spinscale
added a commit
that referenced
this pull request
Nov 27, 2018
The trigger engine did always create a new schedule data structure, when the watcher indexing listener called an add. However the indexing listener also called add, when the watch status was updated. This means, that upon a watch status update the watch got retriggered, potentially waiting a defined interval from the watch status update onwards, instead of waiting from the last run. This commit only updates the schedule in the trigger engine, if it actually has changed, otherwise the existing schedule will not be touched. This has two results 1. If a watch is updated by an execution, the existing interval will not be touched (meaning the scheduled time will not move forward). 2. If a watch is updated by a user, but the schedule is not changed, it will not be reset from the update (for example starting to count from 5 minutes again, if the interval was set to 5 minutes). Furthermore some minor cleanups were applied, making variables final in the ctor, preventing double creation of variables.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
The trigger engine did always create a new schedule data structure, when
the watcher indexing listener called an add. However the indexing
listener also called add, when the watch status was updated. This means,
that upon a watch status update the watch got retriggered, potentially
waiting a defined interval from the watch status update onwards, instead
of waiting from the last run.
This commit only updates the schedule in the trigger engine, if it
actually has changed, otherwise the existing schedule will not be
touched. This has two results
be touched (meaning the scheduled time will not move forward).
will not be reset from the update (for example starting to count from 5
minutes again, if the interval was set to 5 minutes).
Furthermore some minor cleanups were applied, making variables final in
the ctor, preventing double creation of variables, preventing shadow naming.