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

Send version with esql #1841

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Send version with esql #1841

merged 5 commits into from
Apr 16, 2024

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 12, 2024

Elasticsearch version 8.14 will require a version parameter. This defaults it to the first released version. That version is supported in 8.13.3, but not before that.

Elasticsearch version 8.14 will require a `version` parameter. This
defaults it to the first released version. That version is supported in
8.13.3, but not before that.
@nik9000 nik9000 requested review from pquentin and alex-spies April 12, 2024 15:42
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

esrally/driver/runner.py Outdated Show resolved Hide resolved
@pquentin
Copy link
Member

pquentin commented Apr 15, 2024

Do we want to support versions of Elasticsearch that don't know about this parameter? Or at least wait for 8.13.3 to be released?

@nik9000
Copy link
Member Author

nik9000 commented Apr 15, 2024

Do we want to support versions of Elasticsearch that don't know about this parameter? Or at least wait for 8.13.3 to be released?

I haven't checked, elastic/elasticsearch#107438 should make rally work even without this PR and this one can wait until we upgrade.

As for versions of ES.... I dunno! ESQL's not GA in 8.13 so I'd be ok dropping any executions against it. But waiting to 8.13.next seems fine, especially if we have that ES PR.

@gbanasiak
Copy link
Contributor

elastic/elasticsearch#107438 should make rally work even without this PR

Rally uses Python client 8.6.1, so looking at this it won't? I think we need to merge this before version is required on the ES side.

Please also add documentation of version parameter under https://github.com/elastic/rally/blob/master/docs/track.rst#esql.

@alex-spies
Copy link
Contributor

alex-spies commented Apr 16, 2024

As for versions of ES.... I dunno! ESQL's not GA in 8.13 so I'd be ok dropping any executions against it. But waiting to 8.13.next seems fine, especially if we have that ES PR.

++, I'd prefer we do not add more work to make pre-GA ESQL work in benchmarks.

We could make the range of clients that don't need to send the version broader with the approach from elastic/elasticsearch#107438; but that'd be adding more loopholes into the API for pre-GA features.

@alex-spies
Copy link
Contributor

Please also add documentation of version parameter under https://github.com/elastic/rally/blob/master/docs/track.rst#esql.

Added to docs, thanks for the pointer!

@nik9000
Copy link
Member Author

nik9000 commented Apr 16, 2024

run CI / unit macOS 3.8 (pull_request)

Copy link
Contributor

@gbanasiak gbanasiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gbanasiak
Copy link
Contributor

@nik9000 We started seeing benchmark failures with ESQL operations after elastic/elasticsearch#107433 got merged, so it's time to merge this one.

@nik9000 nik9000 merged commit 821f2cc into elastic:master Apr 16, 2024
17 checks passed
@nik9000
Copy link
Member Author

nik9000 commented Apr 16, 2024

Merged!

@nik9000
Copy link
Member Author

nik9000 commented Apr 16, 2024

sorry for the delay

alex-spies added a commit to alex-spies/rally that referenced this pull request May 23, 2024
@alex-spies alex-spies mentioned this pull request May 23, 2024
@gbanasiak gbanasiak added this to the 2.11.0 milestone May 23, 2024
gbanasiak pushed a commit that referenced this pull request May 29, 2024
ES|QL in 8.14 will not require a `version` parameter in requests to `_query`, after all.
This removes the `version` parameter from the ES|QL runner, essentially reverting #1841.
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.

4 participants