villebro commented on PR #36368:
URL: https://github.com/apache/superset/pull/36368#issuecomment-3824450014
Hey @mistercrunch thanks for the review + comments! Many good catches on
missing indexes etc, will clean those up 👍 Regarding the other
comments/questions:
- It's true that the PR only introduces the core plumbing of the feature
without migrating any existing tasks. This has been intentional for the
following reasons:
- The PR is already at 13k LOC, and I feel adding migrations to the PR
would likely divert attention away from the core functionality. The intention
here was to set a solid foundation that makes migrations as painless as
possible once the core functionality is ready. Mind you, I've already done a
few test migrations to validate the proposal, I just didn't want to include
them in this PR.
- As the majority of current long running tasks are missing abort
handlers, they will likely need major refactor during migration. For that
reason I don't want to migrate them as-is, but rather prefer to tackle them one
by one as follow-ups so reviewers can focus on the migrated task at hand.
- I've been doing extensive testing of the feature with some dummy
extensions, so the functionality has been confirmed to work.
- The feature has been built with future-proofing in mind. New
functionality can easily be added to the `task` decorator and/or `TaskOptions`
without requiring migrations or breaking backwards compatibility.
- It was mentioned that Alerts & Reports has it's own table for logging.
That can IMO remain unchanged for now, similar to SQL Lab queries that also
have their own tables and UI. The important thing is to move the execution
logic to the new framework so that tasks can be managed via the new system.
- On the topic of feature flagging and migrating all tasks with a config to
control which tasks get executed on the new framework, I personally feel we
should just migrate them over one by one in quick succession and leverage the
new functionality, rather than having a bunch of unabortable and undeduplicated
tasks executing on the new system. But we can discuss this if you feel strongly
we should have a different rollout strategy.
One more thing that @aminghadersohi brought up about retries: My current
feeling intuition says we should not reinvent the retry wheel in this
framework, but rather provide docs explaining how to implement retries using
`backoff`. But we can have that discussion when we implement the first task
that requires retries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
