fix(alerts&reports): add celery soft timeout support #13436
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13436 +/- ##
==========================================
- Coverage 77.23% 71.42% -5.82%
==========================================
Files 900 807 -93
Lines 45601 40905 -4696
Branches 5415 4210 -1205
==========================================
- Hits 35221 29216 -6005
- Misses 10253 11689 +1436
+ Partials 127 0 -127
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -155,6 +155,10 @@ class AlertQueryError(CommandException): | |||
| message = _("Alert found an error while executing a query.") | |||
|
|
|||
|
|
|||
| class AlertQueryTimeout(CommandException): | |||
| message = _("A timeout occurred while executing the query.") | |||
There was a problem hiding this comment.
As this can happen during either the query or the thumbnail computation, perhaps we should reword this as "A timeout occurred while executing the background job." - is this the same message that would trigger when a Report times out?
There was a problem hiding this comment.
not the same message, this is really specific for queries
There was a problem hiding this comment.
created a specific exception for screenshots timeout also
superset/reports/commands/alert.py
Outdated
| :raises: AlertQueryError, | ||
| AlertQueryInvalidTypeError, | ||
| AlertQueryMultipleColumnsError, | ||
| AlertQueryMultipleRowsError, | ||
| AlertQueryTimeout, | ||
| AlertValidatorConfigError |
There was a problem hiding this comment.
nit: while there are many conventions out there, I believe this is the one we mostly use in Superset:
:raises Exception1: reason1
:raises Exception2: reason2
superset/reports/commands/alert.py
Outdated
| Executes the actual alert SQL query template | ||
|
|
||
| :return: A pandas dataframe | ||
| :raises: AlertQueryTimeout, AlertQueryError |
| except Exception as ex: | ||
| raise ReportScheduleScreenshotFailedError( | ||
| f"Failed taking a screenshot {str(ex)}" | ||
| ) |
There was a problem hiding this comment.
I'm stealing codecov's comment here, but could we add a test for this broad exception case, too?
dpgaspar
deleted the
danielgaspar/ch5697/implement-handling-of-softtimelimit-exception
branch
* fix(alerts&reports): add celery soft timeout support * make a specific exception for screenshots timeout * fix docs, add new test
mistercrunch
added
🏷️ bot
SUMMARY
Handles specifically celery's
SoftTimeoutExceededexception on alerts and reports.On alerts:
On reports:
None, added an extra broad exception that can handleSoftTimeoutExceeded, other exceptions can occur and these were not handled.ADDITIONAL INFORMATION