-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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: Use Celery task ETA for alert/report schedule #24537
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24537 +/- ##
==========================================
+ Coverage 68.93% 69.07% +0.14%
==========================================
Files 1903 1903
Lines 74027 74026 -1
Branches 8118 8118
==========================================
+ Hits 51027 51135 +108
+ Misses 20889 20780 -109
Partials 2111 2111
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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
(cherry picked from commit e402c94)
SUMMARY
After installing Superset with Celery to 5.3.0 (within the defined constraints) we started to notice an issue with Superset trying to parse a date/time string which was already of type
datetime
.I've spent a good amount of time perusing the Celery changelog and couldn't find anything which would have caused the regression, given that it seems we're using JSON for serialization/deserialization and there was no mention of the decoder trying to decode temporal arguments.
I did notice that there was some "magical" handling for the request ETA which, per here, tries to coerce the JSON encoded date/time into a
datetime
object. Digging a little future there seems to be some redundancy in how we define our tasks, i.e., providing both an ETA and a schedule which equate to the same thing.This PR circumvents the problem by simply removing specifying the schedule in addition to the ETA which removes redundancy the need to decode the schedule. Note an alternative would be to tighten the constraint on the upper bound of feasible Celery versions, but this approach seemed more accomodating.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION