Adds a --nocommit arg to the update_index, clear_index and rebuild_index management command. #1090
Conversation
|
@acdha anything missing here? Tests are failing on pypy, but that seems to be because of pypy upgrade on travis-ci side |
|
@iElectric it looks like those tests are failing on all of the recent PRs |
|
indeed. cc @toastdriven |
|
Yeah, Travis appears to have recently upgraded PyPy and since then every build has been failing. |
|
I'm +1 on merging this |
|
@acdha any clue why the test in this patch would fail? Add the same commit flag to --remove of update_index doesn't seem to work either. |
|
@andrewschoen I noticed that both the Whoosh and ElasticSearch backends do not appear to implement commit support for clear(). Whoosh always commits on update and remove: I'm wondering whether this test should actually hit the backends or just do a mock to confirm that backend.clear is called with commit=False |
|
@acdha Solr should support it. You're right, I could try to write a mocked test though. |
|
@andrewschoen Solr definitely does support it – I was wondering whether alternative would be something like a Python warning when the ES/Whoosh/simple backends are called with a non-default commit value in case someone has built logic around commits not happening, but that seems a bit crazy since anything can trigger one. |
|
For posterity, the test code works great but there's an unexpected feature in clear which calls self.conn.optimize, which does a commit: I think we should just wrap that in |
andrewschoen
force-pushed
the
feature/no-commit-flag
branch
from
376e566
to
27f4bbd
Compare
andrewschoen
changed the title
andrewschoen
force-pushed
the
feature/no-commit-flag
branch
from
27f4bbd
to
207685e
Compare
|
This should now work for the update_index, clear_index and rebuild_index commands. I've tried to make the docs clear on what backends support the |
|
@acdha Anything else I need to do to get this ready to merge? |
| @@ -231,9 +235,9 @@ def update_backend(self, label, using): | |||
| end = min(start + batch_size, total) | |||
|
|
|||
| if self.workers == 0: | |||
| do_update(backend, index, qs, start, end, total, self.verbosity) | |||
| do_update(backend, index, qs, start, end, total, self.verbosity, self.commit) | |||
There was a problem hiding this comment.
Could commit (and verbosity) be passed as keyword args here and below to avoid having quite so many positional arguments? Other than that, this looks ready to go.
andrewschoen
force-pushed
the
feature/no-commit-flag
branch
from
207685e
to
c5abef6
Compare
management commands.
andrewschoen
force-pushed
the
feature/no-commit-flag
branch
from
c5abef6
to
6e863c2
Compare
|
@acdha Thanks for the review. I've got those kwargs added to both do_update and do_remove now. |
|
Failures on travis do not seem to be related to this PR. |
|
@acdha @toastdriven anything left to do before merging? |
|
I don't believe so |
|
@acdha let's merge it? cc @toastdriven |
|
Another friendly ping. PR has docs and tests. |
Adds a --nocommit arg to the update_index, clear_index and rebuild_index management command.
|
🍻 |
rebuild_index builds its option list by combining the options from clear_index and update_index. This previously had a manual exclude list for options which were present in both commands to avoid conflicts but the nocommit option wasn't in that list. This wasn't tested because our test suite uses call_command rather than invoking the option parser directly. This commit also adds tests to confirm that --nocommit will actually pass commit=False to clear_index and update_index. Closes django-haystack#1140 See django-haystack#1090
|
This now crashes for some backends, for example xapian, as they do not understand a "commit" argument for the update command. i think "commit" should be passed as an argument only to Solr (who makes sense of it) and not the other backends. Should I open a new bug/patch for this? |
This is helpful when doing bulk indexing and you'd rather have the backend handle commits, not the management command. We had an instance where the amount of hard commits passed by update_index during a bulk index was causing stability issues with our solr cluster that was already doing soft and hard commits often.
I attempted to add this flag to clear_index and the --remove option of update_index, but it seems that solr was not respecting commit=false for those two code paths. I can look into it some more, but wasn't able to find a solution.