Upgrade ES Client to 8.x #1669
Merged
Upgrade ES Client to 8.x #1669
Conversation
This file contains 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
Also add the elastic-transport library as a dependency.
…lity. As of 8.0.0, transport logic has been moved out of `elasticsearch-py` and into a standalone library, `elastic-transport-python`. This includes substantial changes to the lower-level APIs for configuring client objects. This commit adjusts accordingly, but preserves Rally's previous behavior. This commit also reproduces the product verification behavior of `v7.14.0` of the client: https://github.com/elastic/elasticsearch-py/blob/v7.14.0/elasticsearch/transport.py#L606 As of `v8.0.0`, the client determines whether the server is Elasticsearch by checking whether HTTP responses contain the `X-elastic-product` header. If they do not, it raises an `UnsupportedProductException`. This header was only introduced in Elasticsearch `7.14.0`, however, so the client will consider any version of ES prior to `7.14.0` unsupported due to responses not including it. Because Rally needs to support versions of ES >= 6.8.0, we resurrect this earlier logic for determining the authenticity of the server, which does not rely exclusively on this header.
The client's `transport` object will no longer have a `hosts` attribute, but will had a `node_pool` attribute. Exceptions are also different-- a TransportError will not have a `status_code` attribute, but should contain a status code as its `message`.
The ES client now requires kwargs, so we ensure that we do not use any positional arguments. The client also now distinguishes between a `TransportError` and an `ApiError`, and `ElasticsearchException` has been removed. We adjust accordingly, albeit with a TODO to revisit this.
This migrates to the new `TransportError`/`ApiError` approach, but it's a work in progress and needs another pass before it's considered robust.
This is the new, preferred way of doing this, and these requests fail otherwise. We should genericize this behavior across runners.
Using ssl= is the preferred way since 3.0 (aio-libs/aiohttp#2626) and ssl_context= goes away in the next version, 4.0 (aio-libs/aiohttp#3548).
…ass, and perform verification after the target request
|
I re-ran the -Median Throughput | bulk-index | 22988.9 | 22772.2 | -216.708 | docs/s | -0.94% |To further confirm I compared the bulk-index throughput to the nightly benchmarks over the last 7 weeks: |
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
This reverts commit a16450d.
There was a problem hiding this comment.
This is really great work @b-deam.
One question/suggestion: Do you think it's worth running a CCS benchmark with this PR? A priori, I don't think there would be any issues there, but instantiating clients for multiple clusters is a code path that isn't otherwise exercised by the testing you've done, so it may be worth a sanity check.
Good suggestion. I tested the CCS challenge locally, but for completeness' sake I ran a comparison using the nightly configuration. The results look OK to me. ccsI also tested the many-shards-snapshotsbaseline: 6447ae1f-f5d6-438e-835b-90b58bbd403a |
|
@elasticmachine run rally/it-python38 |
Great to see this tech debt item fixed as well via this PR/upgrade. |
I doubt that the 20% increase in the how long we wait for the snapshots to complete is due to b9808fc and switching to the native API call We need to keep an eye on the results in https://elasticsearch-benchmarks.elastic.co/#tracks/many-shards-snapshots/nightly/default/90d after merging. Unfortunately we've been hitting some random unexplainable variability lately. |
michaelbaamonde
approved these changes
Mar 2, 2023
pquentin
added a commit
that referenced
this pull request
Mar 4, 2023
b-deam
added a commit
that referenced
this pull request
Mar 5, 2023
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.
Note for reviewers:
bodyparameter in client methods is pervasive across the code base, luckily for us the 8.6 release has an internal decorator that rewrites these params and logs a deprecation warningrequest_timeoutparam is now a transport option that must be set via the client's respectiveoptions()method (e.g.es.options(request_timeout=..).search(..)). Again, luckily for us the client rewrites this parameter and sets the corresponding transport option and logs a deprecation warningA high level overview of my plan:
esrally race [...]against:--client-optionsTODOs in thees-client-upgradebranchelasticsearch[async]andelastic-transportlibs to8.6.1and8.4.0, respectivelybodyparameter to properly typed/named parameters.options()for transport parameters #1673 to track the refactoring ofrequest-timeoutelastic/logsis a given, because of its pervasive nature in a lot of our workgeonames,nyc_taxisetc.)Closes #1350