Add options to index celery tasks in the host queue #4459
Conversation
This lets us run async celery tasks in prod, but only on one machine that is setup for the new ES.
safwanrahman
force-pushed
the
search-deploy-additions
branch
from
ca02c48
to
b0e51cc
Compare
|
@ericholscher I have cherry picked the commit and forced push to your branch. Now it looks good. |
| @@ -32,7 +33,13 @@ def _get_indexing_tasks(app_label, model_name, queryset, document_class, index_n | |||
| } | |||
| yield index_objects_to_es.si(**data) | |||
|
|
|||
| def _run_reindex_tasks(self, models): | |||
| def _run_reindex_tasks(self, models, host_queue): | |||
| apply_async_kwargs = {'priority': 0} | |||
There was a problem hiding this comment.
I dont think priority is necessary.
|
@ericholscher I think if you want to run the command in the same |
|
|
|
Looks good other than the priority one. r+ |
| parser.add_argument( | ||
| '--host-queue', | ||
| dest='host_queue', | ||
| action='store_true', |
There was a problem hiding this comment.
I think it should be store only since it's a string what we want here.
| @@ -32,7 +33,13 @@ def _get_indexing_tasks(app_label, model_name, queryset, document_class, index_n | |||
| } | |||
| yield index_objects_to_es.si(**data) | |||
|
|
|||
| def _run_reindex_tasks(self, models): | |||
| def _run_reindex_tasks(self, models, host_queue): | |||
There was a problem hiding this comment.
I'd use a default value here host_queue=None.
There was a problem hiding this comment.
Looks like we should default False here, the parser option is boolean default False
There was a problem hiding this comment.
I think that None makes more sense since we are expecting a str not a bool and the parser option should also be changed to None.
There was a problem hiding this comment.
Both of these points are expecting host_queue to be a bool though, so the logic so far stands.
It would be more work to run the command, but probably less prone to breakage if we don't auto-determine the queue name. In that case, we shouldn't have a default at all and should require the user enters a specific queue name.
agjohnson
added
the
PR: work in progress
|
This commit is added in #4636. so closing! |
This lets us run async celery tasks in prod, but only on one machine that
is setup for the new ES.
This also pulled in
masterwith the merge, but the only commit to look at is ca02c48