-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Nuke Celery #4521
Nuke Celery #4521
Conversation
@rauchy For sure I’ll take a look tomorrow when I’m back at the computer. I’m happy to provide code to replace the periodic tasks with RQ ones, this is a pretty important API for us. Thanks for reaching out! |
@jezdez great, thank you! If you don't mind, I think it might be better to submit it as a standalone PR. |
|
||
return json_response(response) | ||
|
||
|
||
@routes.route("/api/admin/queries/rq_status", methods=["GET"]) |
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.
Maybe we should switch the URLs not to leak the implementation detail (RQ) now that it's only RQ?
@@ -18,9 +14,6 @@ | |||
redis_connection, | |||
rq_redis_connection, | |||
) | |||
from redash.metrics import celery as celery_metrics # noqa |
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.
Need to remove redash/metrics/celery.py
as well.
@rauchy in your opinion, anything blocking us from merging this? |
What type of PR is this? (check all applicable)
Description
Following #4413, we're 100% on RQ. This PR is strictly cleanup.
@jezdez - I'd love to get some context / suggestions from you regarding how to proceed with the periodic tasks. I didn't mean to be rude when I deleted them in this PR, I just wanted to remove as much as possible.
Related Tickets & Documents
#4413 #4092