[#2977] Basic implementation of background jobs via python-rq #3165
Conversation
|
You rock @torfsen 🍻 Don't put anything on Are you envisaging Redis being a hard requirement? |
|
I think we all agreed to accept Redis as a hard requirement when this work is merged |
|
Yup. It will also allow us to use Flask-Cache/plain redis as a general cache in core CKAN instead of stuffing everything into Solr. |
|
@amercader I've moved the connection test to |
torfsen
force-pushed
the
2977-rq-background-tasks
branch
from
f68d434 to
c6fcb06
Compare
|
To Do:
|
|
@TkTech You mentioned that you want to use Redis for other parts of CKAN, too. How should we let the user configure the Redis connection? Since we're going to use 3rd-party code that uses Redis we probably want to put each distinct usage (background jobs, Flask cache) into a separate Redis database to make sure that they don't interfere with each other. Hence I suggest that we only add a single configuration option, |
|
We need to support it all in a single database. People using hosted redis, or odd hosting setups may not be able to use There's no problem allowing all our uses to live in one database so long as everything is prefixed. |
|
@TkTech Good point regarding hosted Redis. RQ prefixes its keys, so interference should be avoidable. It also seems to empty queues intelligently. However, it seems that the prefix is not configurable, so we cannot add the CKAN site ID. This means that two CKAN instances can share a Redis instance but must use separate databases. We still have to be cautious, though: For example, ckanext-harvest simply flushes the whole Redis database to empty its queues. This is a trivial example, since we control that code, but we must be careful not to miss something like that in 3rd-party code. |
|
https://github.com/nvie/rq/blob/766bb600060c85e8a39579541798930bbb508ec8/rq/queue.py#L31 Looks like we can "configure" the prefix this way. We should definitely update the plugins we control (no good plugin should be wiping the entire keyspace! Always use prefixes in redis), but for 3rd party plugins we shouldn't consider that our problem. Document it and assume good behavior. |
|
@TkTech Unfortunately there doesn't seem to be such a setting for the prefix of job keys, that seems to be hard-coded. Update: There's a PR for that feature: rq/rq#685. |
|
It just occurred to me that we can also simply use the CKAN site ID as the queue name for RQ. |
|
Regarding compatibility with ckanext-harvest: See ckan/ckanext-harvest#257 and the corresponding PR ckan/ckanext-harvest#258. |
|
Do we need support for multiple queues? #2706 had, and ckanext-harvest also uses two separate queues (gather and fetch). This PR currently only supports a single queue but it would be easy to support an arbitrary number of named queues instead. Workers could then listen on all or a certain subset of the queues. @rossjones and @amercader, what was the rationale behind the multiple queue-functionality in #2706 and ckanext-harvest, respectively? |
|
I had multiple queues in that PR because on data.gov.uk we use different queues for different priority tasks. That way we can have a busy low-priority queue, but still get out the small tasks pretty quickly. FWIW I'd support multiple queues if it wasn't much work, but I wouldn't let it hold you up. |
torfsen
force-pushed
the
2977-rq-background-tasks
branch
3 times, most recently
from
917fe7b to
3f5b46f
Compare
|
Multiple queues are now supported. By default there is just a single queue ( Queue names are automatically prefixed with the site ID so multiple CKAN instances can now share a Redis database. |
torfsen
force-pushed
the
2977-rq-background-tasks
branch
8 times, most recently
from
4472541 to
2aadcb6
Compare
This output doesn't go into the CKAN logs but into a separate worker log (`/var/log/ckan-worker.log` if the default Supervisor configuration is used).
In most cases one can simply use `tempfile.NamedTemporaryFile` instead.
Mention that it's usually best to simply use the `ckan.tests.recorded_logs` context manager.
The documentation previously talked about sharing databases which could be misunderstood as the possibility of sharing PostgreSQL databases between CKAN instances (instead of sharing Redis databases, which was the intended meaning).
A previous commit for ckan#2977 removed `ckan/config/celery-supervisor.conf` as part of deprecating the old Celery background task system. However, the old documentation told people to copy *or link* that file, so removing it could break existing installations. Hence this commit restores the file, it should be kept around until support for the Celery system is removed completely.
torfsen
force-pushed
the
2977-rq-background-tasks
branch
from
5a21108 to
18e140b
Compare
|
Thanks for the review and good idea regarding I've also restored |
| the name of your extension. For example: | ||
|
|
||
| * The names of *configuration settings* introduced by your extension should | ||
| have the form ``ckan.my_extension.my_config_setting``. |
There was a problem hiding this comment.
The two patterns I've seen for configuration options are
ckanext.my_extension.my_config_setting = ...
and
my_extension.my_config_setting = ...
I choose the latter for my extensions because I find the configuration options are really long with the former.
Anything starting with ckan. should belong to ckan core.
There was a problem hiding this comment.
I've changed it to my_extension.my_config_setting.
The docs now suggest to use `my_extension.my_setting` instead of the previously suggested `ckan.my_extension.my_setting`.
| prefixed names. Use the functions ``add_queue_name_prefix`` and | ||
| ``remove_queue_name_prefix`` to manage queue name prefixes. | ||
|
|
||
| .. versionadded:: 2.6 |
|
|
||
| import logging | ||
|
|
||
| from pylons import config |
There was a problem hiding this comment.
Since #3163 was merged we should never import config directly from Pylons anymore. Use from ckan.common import config.
My fault for not documenting / announcing it
|
@torfsen This looks really, really good. Thanks for being so thorough with the tests and docs. Apart from the minor comments added it looks good to me. Can't wait to try it with ckanext-harvest! |
|
any objections to merging this now? |
|
No, let's get it in! |
|
Yay, great work everybody! |
|
@torfsen I wrote a tiny stub of an extension that uses this code https://github.com/wardi/ckanext-geocodejob but it seems rq's forking is interfering with ckan's sqlalchemy state. My first job works but jobs after that fail with Can we wrap the job running with code that cleanly closes our database connections? |
This PR is WIP for #2977, a new implementation of background jobs using python-rq.
(Note that I'm not interested in the bounty)
It is loosely based on #2706 by @rossjones.
The implementation has a minimal feature set on purpose: I'm fine with adding more features but I wanted to avoid introducing things that nobody needs. So if you feel that this PR is missing an important feature please speak up.
Likewise, documentation and tests will not be written until there is an agreement regarding the general architecture.
There is now the
ckan.lib.jobsmodule which most importantly provides theinit_queueandenqueuefunctions. The former is called during CKAN startup and establishes the connection to Redis. I wanted to make sure that we fail early when Redis is not available (and not later at some random time when a job is enqueued). However I'm not sure whetherckan/websetup.pyis the right place for that -- feedback appreciated!Once you have enqueued a job using
ckan.lib.jobs.enqueueyou can use thejob_*API functions to manage the queue:job_list: List all enqueued jobsjob_show: Show details for a certain jobjob_cancel: Cancel a certain enqueued jobjob_clear: Clear the whole queueSimilar functionality is provided by
paster jobs, see its help text. In particular,paster jobs workerstarts a worker that fetches jobs from the queue and executes them. If you want to play around you can usepaster jobs testto enqueue test jobs (which you then can list, cancel, execute, etc.).