Execute Queries in RQ #4413
Execute Queries in RQ #4413
Conversation
…alization clearer
redash/tasks/queries/execution.py
Outdated
| 'scheduled': scheduled_query_id is not None, | ||
| 'query_id': metadata.get('Query ID'), | ||
| 'user_id': user_id | ||
| }) |
There was a problem hiding this comment.
Btw, does the RQ API let us segment tasks by the same dimensions (org_id, data_source_id, query_id, user_id)?
redash/tasks/queries/execution.py
Outdated
| else: | ||
| self._async_result = AsyncResult(job_id, app=celery) | ||
| def __init__(self, job): | ||
| self._job = job if isinstance(job, Job) else CancellableJob.fetch(job, connection=rq_redis_connection) |
There was a problem hiding this comment.
Maybe class method to create them to avoid confusion and weird issues? (from_job_id and from_job)
redash/tasks/queries/execution.py
Outdated
|
|
||
| if isinstance(result, (TimeLimitExceeded, SoftTimeLimitExceeded)): | ||
| result = self._job.result | ||
| if isinstance(result, JobTimeoutException): |
There was a problem hiding this comment.
I'm not sure we still return this one.
| @@ -111,10 +109,10 @@ def enqueue_query(query, data_source, user_id, is_api_key=False, scheduled_query | |||
| if job_id: | |||
There was a problem hiding this comment.
I know I said I’ll port enqueue_query from Celery to RQ as is and deal with refactoring later, but I’m really tempted to use RQ’s ability to set custom IDs to jobs and get rid of the whole lock mechanism using it (i.e. job IDs will be rq:job:<ds.id>:<query_hash> ). WDYT @arikfr?
There was a problem hiding this comment.
This is indeed tempting! But --
- We lose ability to track down specific invocation (in logs and such), because every invocation has the same id.
- Current hash implementation needs fixing (Case insensitive parameters in Redash query result cache #2137).
- Are we sure job IDs were meant to be used this way?
There was a problem hiding this comment.
- That could be easily solved by a custom description or meta attribute holding an invocation counter.
- That needs fixing regardless, but does that affect whether we should drop locks?
- It's not a hack, if that's what you mean. RQ docs specify that you can use a predetermined ID instead of the auto-generated one.
There was a problem hiding this comment.
RQ docs specify that you can use a predetermined ID instead of the auto-generated one.
But do they expect the ID to be non unique/reusable? An execution's id feels like something that supposed to be unique, I'm just concerned about the can of worms we're opening if we assign a non unique one.
There was a problem hiding this comment.
All jobs scheduled from rq-scheduler reuse the same identifier :)
There was a problem hiding this comment.
You might want to check out: rq/rq#793.
There was a problem hiding this comment.
TL;DR:
What sirex said, the job will be executed twice (if the job is still in Redis).
There was a problem hiding this comment.
I fail to see the issue. We still check for the job's existence in Redis. We get the same effect of a lock by checking if the job is queued. We just don't have to worry about maintaining another key and expiring it.
There was a problem hiding this comment.
I thought you were suggesting reusing job ids to avoid having to implement our own lock.
While we don't need to worry about another key, we do need to worry about lots of other things and fully understand how rq works. Considering that the current implementation works and we have reasonable processes for it, I don't think we will gain much here at this stage, until we're more familiar with RQ. The risk vs. gain is just not worth it.
This reverts commit 37a74ea.
| @@ -26,75 +27,6 @@ def _unlock(query_hash, data_source_id): | |||
| redis_connection.delete(_job_lock_id(query_hash, data_source_id)) | |||
|
|
|||
|
|
|||
| class QueryTask(object): | |||
There was a problem hiding this comment.
👋 good bye old friend. This class survived since the very first iterations of Redash (it was actually named Job back then), when we had my half-assed background job implementation and Tornado as the web server.
After too much frolicking, I realized that setting this at the start of the app doesn't play nice with Flask's threads and RQ's LocalStack. I'll keep the pre-request & post-request for now unless you can see any fundamental issues with it. |
Do we really push a request context when starting the app and/or executing a job? |
|
@arikfr which query runners do you think I should try this on? |
What type of PR is this? (check all applicable)
Description
Moves query executions from Celery to RQ.
I was really tempted to clean up the code as I went along with this refactoring, but I decided to move things as they are and get them working on RQ, then simplify.
Related Tickets & Documents
#4307