Improved support for refresh during updates #323
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Elasticsearch bulk APIs support a configurable 'refresh' parameter that can be set to: 1. empty string / true : perform refresh after operation completes 2. 'wait_for': wait until next scheduled refresh 3. false (default): don't do any refresh See documentation at: https://www.elastic.co/guide/en/elasticsearch/reference/master/docs-refresh.html Currently the `Document.update()` API accepts a `refresh` parameter that, if `True`, passes `True` to the ES bulk API. There isn't currently a way to pass 'wait_for'. This commit modifies how `Document.update()` works so that it is possible to pass `refresh='wait_for'`. I've implemented this in a generic way, as opposed to hardcoding logic for 'wait_for', to support future changes to this ES bulk API. Note that, in keeping with the current convention, if `refresh='wait_for'` is passed, this will override the value of auto_refresh set either via setting or via the Document class. This gives developers the maximum configurability here.
Currently when updating or rebuilding an index, the `refresh` parameter is never set, regardless if settings.ELASTICSEARCH_DSL_AUTO_REFRESH is set or if auto_refresh is set on the Document class. This commit adds the ability to pass `--refresh` to the `search_index` management command, to force a refresh when the index is updated. This can be useful, for example, if you want to programmatically call that command from some Django tests that fill an index, and then immediately run tests against that index. Without setting `refresh=True`, there's no guarantee that the refreshed index will be available to search.
chosak
force-pushed
the
feature/refresh-improvements
branch
from
abb42d2 to
b22d27a
Compare
|
@chosak Thanks a lot for the PR. I think it was better to create a issue before submitting a patch. So we could have discussed how this could be implemented! As you have submitted the patch, I will review it and will surely try to merge it to master. Thanks |
safwanrahman
reviewed
Mar 6, 2021
safwanrahman
reviewed
Mar 6, 2021
|
@chosak I have couple of simple issue about this PR, other than that it looks really good. Can you fix that so we can get it merged? |
|
👍 @safwanrahman thanks for the review! |
safwanrahman
reviewed
Mar 7, 2021
safwanrahman
approved these changes
Mar 9, 2021
chosak
added a commit
to chosak/consumerfinance.gov
that referenced
this pull request
Apr 22, 2021
This commit upgrades our use of the django-elasticsearch-dsl package from version 7.1.4 to new version 7.2.0: https://github.com/django-es/django-elasticsearch-dsl/releases/tag/7.2.0 This allows us to remove some patching code in Elasticsearch tests related to refreshing indexes deterministically after an update, the result of this PR: django-es/django-elasticsearch-dsl#323
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes two commits that both improve the ability to refresh during updates.
First, it adds the ability to pass
refresh='wait_for'toDocument.update(). Currently you can either pass nothing (in which case no refresh happens) orrefresh=True, in which case the refresh happens right away. Per the docs, it's also possible to specifyrefresh='wait_for'toSecond, it adds a new
--refreshparameter to the search_index management command. Currently when running that command, there's no way to force a refresh to happen. This can be useful, for example, if you want to programmatically call the command from some Django tests that fill an index, and then immediately run tests against that index. Without settingrefresh=True, there's no guarantee that the refreshed index will be available to search.See commit messages for additional details.