[CRE-6175] routed trigger event - #23608
Conversation
…h an event keeps the expiry date/time even when changing the timeout settings
|
👋 agparadiso, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
|
|
|
||
| // Stamp the deadline once at dispatch: observedAt + queue timeout. | ||
| // If ObservedAt is not set, use the current time. | ||
| if event.ObservedAt.IsZero() { |
There was a problem hiding this comment.
@agparadiso What is the correct behaviour here? I would expect that we control both sides of the interface here, inside two components of the core node -- i.e. we should either always be setting it, or never set it here.
If it's missing when it shouldn't be, let's treat it as a validation error.
| if event.ObservedAt.IsZero() { | ||
| event.ObservedAt = e.cfg.Clock.Now() | ||
| } | ||
| queueTimeout, err := e.cfg.LocalLimiters.TriggerEventQueueTime.Limit(ctx) |
There was a problem hiding this comment.
Nit: consider calling this ...QueueTimeout to be consistent with the variable name
3 participants
Description
This pr implements the logic around the
deadlinefield. Before this change the way we used to determine if an event was expired was to calculate the elapsed time and compare it against the queue timeout settings at dequeue time. This implies that an expired event consumes queue resources, makes the rest wait to then be discarded. It also means that an event that was already enqueue with a particular timeout expectation might be expired if the settings changes before the dequeue.Thats the main reason of this change, to set a deadline per event, so no settings changes can affect what "was agreed" at the beginning. We could potentially also define different deadlines to different org events.
CRE-6175
Requires
Supports