-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix: Stabilize and deprecate legacy alerts module #12627
fix: Stabilize and deprecate legacy alerts module #12627
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12627 +/- ##
==========================================
- Coverage 66.79% 66.72% -0.07%
==========================================
Files 1015 1017 +2
Lines 49673 49734 +61
Branches 4845 4864 +19
==========================================
+ Hits 33179 33187 +8
- Misses 16372 16424 +52
- Partials 122 123 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 just a non blocking comment
@@ -844,8 +846,8 @@ def schedule_alerts() -> None: | |||
resolution = 0 | |||
now = datetime.utcnow() | |||
start_at = now - timedelta( | |||
seconds=3600 | |||
) # process any missed tasks in the past hour | |||
seconds=300 |
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.
would be nice to have this on a config flag
* Add deprecation notice to superset.tasks.schedules * Reduce retries and schedule window for alerts (cherry picked from commit e4ae17d)
* Add deprecation notice to superset.tasks.schedules * Reduce retries and schedule window for alerts (cherry picked from commit e4ae17d)
SUMMARY
We are seeing exponential growth of
alert.run_query
tasks in our Celery queues, likely due to the permissive scheduling window combined with multiple task retries. This PR 1) reduces the schedule window from 1hr to a few minutes, 2) reduces the retry count from 5 to 1, and 3) removes the arbitrarysoft_time_limit
value.Also adding a deprecation notice to this module, as it's been replaced by #11711
TEST PLAN
Alert scheduling should function without exponential growth of celery task queues.
ADDITIONAL INFORMATION