-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RAM] Garbage collect expired snoozes #135271
Conversation
return { | ||
id, | ||
snoozeEndTime: new Date(MAX_TIMESTAMP), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file is just a copy-paste of code removed from isRuleSnoozed
, except for this check for duration === -1
I'm temporarily having an indefinite snooze return an end time of MAX_TIMESTAMP
just to avoid type errors and allow snoozes to be sorted by duration effectively. This timestamp is never parsed in the code because the UI and task runner still rely on the muteAll
property. When we take care of #132266 we can look into whether passing MAX_TIMESTAMP
for indefinite snoozes is the right thing to do, or if we want to handle this case differently.
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only added tests for isSnoozeExpired
because it's new code. isSnoozeActive
is existing code moved to a new module, and all of its functionality is covered very robustly by the tests for isRuleSnoozed
. IMO adding additional tests for isSnoozeActive
would be redundant, unless we similarly pulled them out of the isRuleSnoozed
tests.
Pinging @elastic/response-ops (Team:ResponseOps) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! also love the tests, just had a question about a failure case potentially.
} | ||
return null; | ||
}) | ||
.map((snooze) => isSnoozeActive(snooze)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
); | ||
|
||
const snoozeSchedule = attributes.snoozeSchedule | ||
? attributes.snoozeSchedule.filter((s) => !isSnoozeExpired(s)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like isSnoozeExpired
can throw, just curious to know what would happen if it does throw? I guess 1 invalid snooze could potentially prevent all other snoozes from being cleared. Maybe we should have some kind of recovery logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #134726
Every time the task manager evaluates a rule, it will clear any expired snoozes from its
snoozeSchedule
. This solves the issue of expired snoozes being visible in the UI.Checklist