Merged
Conversation
joamaki
approved these changes
Aug 18, 2024
Add WithDebounce functional option to the timer job trigger. This allows to fold multiple triggers over the specified interval. Also, it is now possible to have a timer job with a zero interval, that runs its function only when explicitly triggered. This replicates the behavior of the github.com/cilium/cilium/pkg/trigger package in the Cilium codebase, making the same semantic available within a job from the hive framework. Note that the debounce is striclty tied to the trigger and does not affect the behavior of the timer job when the internal timer expires. In that case, the job is run without taking into account the debounce interval. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Add support for reporting trigger statistics in job related metrics. Specifically, the trigger reports the latency between the first trigger and the next execution of the function, alongside the number of coalesced triggers. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
pippolo84
force-pushed
the
pr/pippolo84/debounce
branch
from
780df3d to
0be55ca
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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 current Trigger implementation does not allow folding multiple requests over a time interval. In the cilium codebase, this feature is available in the "github.com/cilium/cilium/pkg/trigger" package (see cilium/cilium#32973 (comment) as an example) but lacks the benefits of the hive integration.
The PR adds the ability to specify a debounce interval to a timer trigger. Doing so, multiple trigger requests over the debounce interval are folded and the trigger is called just once. A debouncing trigger can work with:
The job metrics interface is extended to make the trigger statistics available to the consumers. Right now, the trigger exports the latency between the first trigger and the actual job run, alongside the number of folded triggers in a debounce interval.
Notes about alternative implementations
What I dislike about this approach is the possible confusion around the already existent
WithTriggeroption for the timer job and the new trigger job. This could have been avoided removing the trigger option for the timer, but apart from breaking the package public interface with a non backward compatible change, it would have removed a nice feature of the Timer job.Overall, the changes needed to implement the new semantic seem not too difficult and consistent, so I think extending the timer is better.