fix: use nullpool in the celery workers #10819
Conversation
|
Was this confirmed to fix the error? There have been similar errors reported for email alerts, so pretty awesome if this solves these problems completely. |
bkyryliuk
force-pushed
the
bogdan/nullpool_celery
branch
2 times, most recently
from
e27d728
to
c60cdf4
Compare
@villebro still iterating on it, will post an update once confirmed |
bkyryliuk
force-pushed
the
bogdan/nullpool_celery
branch
from
c60cdf4
to
1036e9c
Compare
Codecov Report
@@ Coverage Diff @@
## master #10819 +/- ##
==========================================
- Coverage 65.40% 61.35% -4.05%
==========================================
Files 802 804 +2
Lines 37872 37936 +64
Branches 3561 3561
==========================================
- Hits 24770 23277 -1493
- Misses 12996 14473 +1477
- Partials 106 186 +80
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
bkyryliuk
force-pushed
the
bogdan/nullpool_celery
branch
2 times, most recently
from
dd30315
to
a9be817
Compare
bkyryliuk
force-pushed
the
bogdan/nullpool_celery
branch
from
a9be817
to
8f2f6eb
Compare
@villebro I've confirmed that this fix is working for us, I've ran alerts and reports over night with high frequency & the issue has stopped happening |
bkyryliuk
changed the title
superset/cli.py
Outdated
| with session_scope(nullpool=True) as session: | ||
| schedule_window( | ||
| ScheduleType.alert, | ||
| datetime.now() - timedelta(1000), | ||
| datetime.now(), | ||
| 6000, | ||
| session, | ||
| ) |
There was a problem hiding this comment.
Not yours, but I usually like using named arguments in cases like this to make stuff more readable. 6000 what?
| from superset.models.alerts import Alert, SQLObservation | ||
| from superset.sql_parse import ParsedQuery | ||
|
|
||
| logger = logging.getLogger("tasks.email_reports") | ||
|
|
||
|
|
||
| def observe(alert_id: int) -> Optional[str]: | ||
| def observe(alert_id: int, session: Session) -> Optional[str]: |
There was a problem hiding this comment.
I think the idea was to mostly remove references to sessions in sigs (there's lots of them scattered around), as they can usually just be picked up from db, so it might be a good idea to add a comment here that this really needs to be passed along here.
There was a problem hiding this comment.
good point, added comments and issue reference
* Use nullpool in the celery workers * Address comments Co-authored-by: bogdan kyryliuk <[email protected]>
mistercrunch
added
🏷️ bot
SUMMARY
Changes celery db requests from superset.db to the scoped_session that uses null pool to avoid using same db sessions across multiple celery workers.
More context on the problem can be found in the linked issue.
Fixes #10530
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION