Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare 7.0.1 release with skipped ES cluster version memoization for search requests #792

Closed
wants to merge 1 commit into from

Conversation

rabotyaga
Copy link
Contributor

@rabotyaga rabotyaga commented May 3, 2021

The “total hits” counter is an integer for ES versions < 7 and an object (hash) for the versions starting from 7.0.0. Elasticsearch added a special option, rest_total_hits_as_int, to ease the upgrade, that could be appended to any request and results in the old “total hits” format. Unfortunately, this option is not recognized by ES versions prior to 7.0.0, which means that we have to check the version to decide if we need this option.

Normally Chewy does memoization of the current ES version, but this might be inappropriate for the upgrade, as the version changes live.

Release the v7.0.1 Chewy version to allow dynamically detect the current Elasticsearch version.
More details in the Migration Guide.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • (n/a) Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • (n/a) Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

@rabotyaga rabotyaga marked this pull request as ready for review May 3, 2021 12:04
@dalthon
Copy link
Contributor

dalthon commented May 3, 2021

@rabotyaga, don't you think that it would be nice to keep this memoization by default and only disable it with some configuration?
The issue you described seem to be a very rare case that in order to be handled puts an extra burden in all requests

@rabotyaga
Copy link
Contributor Author

@rabotyaga, don't you think that it would be nice to keep this memoization by default and only disable it with some configuration?
The issue you described seem to be a very rare case that in order to be handled puts an extra burden in all requests

@dalthon You're absolutely right. And this configuration is release 7.0.1. Please take a look at https://github.com/toptal/chewy/pull/792/files#diff-134d08723ee8d2712d2f3250f8936bda26c620b022ec5d7003b3c06b114938c0R20.

@rabotyaga
Copy link
Contributor Author

We don't need to actually merge this, the release is published from the tag: https://github.com/toptal/chewy/releases/tag/v7.0.1

@rabotyaga rabotyaga closed this May 3, 2021
@rabotyaga rabotyaga deleted the 7.0.1-branch branch May 3, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants