Skip to content

add elasticsearch 8.x.x support#244

Open
ckutlu wants to merge 13 commits into
ionelmc:masterfrom
ckutlu:feature/support_es_api_8.x.x
Open

add elasticsearch 8.x.x support#244
ckutlu wants to merge 13 commits into
ionelmc:masterfrom
ckutlu:feature/support_es_api_8.x.x

Conversation

@ckutlu

@ckutlu ckutlu commented Jul 4, 2023

Copy link
Copy Markdown

Elasticsearch python api deprecates the doc_type argument in the index function in v8.x.x. This is a minor fix for that.

Comment thread src/pytest_benchmark/storage/elasticsearch.py Outdated
@ckutlu ckutlu force-pushed the feature/support_es_api_8.x.x branch from 61c8961 to cbeb977 Compare December 16, 2023 22:18
@ckutlu

ckutlu commented Dec 17, 2023

Copy link
Copy Markdown
Author

I rebased this one on recent master and added stuff to authors and changelog as suggested in the contribution guide. Ran tox locally, and it seems to pass (output attached), but not sure if it ran the full suite as expected. I am not really familiar with tox. Is there anything I am missing @ionelmc ?

tox_output.log

@ionelmc

ionelmc commented Dec 17, 2023

Copy link
Copy Markdown
Owner

How that I think of this more, considering that doc types are completely optional since es7, and I don't want to support really old stuff, I think the better way to do this is to completely remove doc_type and just require es7 as the minimum client version.

@ckutlu

ckutlu commented Dec 18, 2023

Copy link
Copy Markdown
Author

How that I think of this more, considering that doc types are completely optional since es7, and I don't want to support really old stuff, I think the better way to do this is to completely remove doc_type and just require es7 as the minimum client version.

Makes sense. I can constrain the elastic version to > 7 and make changes to remove supplying doc_type argument altogether in this MR then. Are you okay with it?

@ionelmc

ionelmc commented Dec 18, 2023 via email

Copy link
Copy Markdown
Owner

@ckutlu

ckutlu commented Dec 27, 2023

Copy link
Copy Markdown
Author

Sorry for the delay, just got around to doing this. Following the discussion in elastic/elasticsearch-py#1698, I went with dropping support for below 7.15.0. Removed most of the version checks I introduced and simplified the changes. There is still a version check for ES 7 in one place. This was needed because 8.x deprecates the ignore keyword on .indices.create (https://www.elastic.co/guide/en/elasticsearch/client/python-api/current/release-notes.html#_deprecated). I tested index creation manually using a 7.17.0 and 8.11.0 version elasticsearch python client against a locally running elastic 8.11 instance. Anything I might have missed @ionelmc ?

@ckutlu

ckutlu commented Dec 28, 2023

Copy link
Copy Markdown
Author
  • Fixed formatting (for check)
  • Changed the unreachable author address of Marc Abramowitz to point to his github.
  • Removed tox environment for pypy37. Fixes the environment build failure due to cryptography not having (anymore?) support for pypy3.7 (here it says they support PyPy3 7.3.10+ which is essentially python 3.8+.)

@ckutlu

ckutlu commented Jan 5, 2024

Copy link
Copy Markdown
Author

This should be now hopefully good to go @ionelmc

@ckutlu

ckutlu commented Jan 7, 2024

Copy link
Copy Markdown
Author

🤦 Argh, failed again. Okay, so there were two problems:

  1. I forgot to run bootstrap.py to get rid of pypy37 actions from the workflow. This should be fixed now.
  2. py312-pytest73-nodist-cover (windows) failed due to hitting the github action timeout of 30 minutes.

I am not sure how to go with the 2nd issue. 30 minutes should be more than enough given that py311-pytest73-nodist-cover (windows) runs in 8 minutes. Somehow things become 4x slower with Python 3.12. We could either increase the timeout a bit (40 min?), or maybe get rid of pytest 7.3 support since py312-pytest7.4 seem to complete within 25 min. What do you think @ionelmc ?

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