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.

Sign up for GitHub

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

Jump to bottom

Remove esql version #1849

Merged
merged 2 commits into from
May 29, 2024
Merged

Remove esql version #1849

merged 2 commits into from
May 29, 2024

Conversation

alex-spies
Copy link
Contributor

ES|QL in 8.14 will not require a version parameter in requests to _query, after all.

Remove the version parameter from the ES|QL runner, essentially reverting #1841.

This should be merged only after elastic/elasticsearch#108919 is merged to avoid breakage.

@gbanasiak
Copy link
Contributor

The last released Rally version 2.10.0 does not send the version and we are about to release version 2.11.0. I think we should refrain from releasing 2.11.0 until we remove version. Does that sound reasonable to you @alex-spies ?

The IT tests are failing which is expected. Once elastic/elasticsearch#108919 is merged they should pass.

@gbanasiak
Copy link
Contributor

gbanasiak commented May 23, 2024 •
edited
Loading

Also, if we ever have to investigate ES|QL regression in the range of ES commits where version is required, we will have to use specific Rally commit ID (from before this PR), which is not a major problem.

@gbanasiak gbanasiak mentioned this pull request May 23, 2024
Merged
@alex-spies
Copy link
Contributor Author

Thanks for your comments @gbanasiak , very good points.

The last released Rally version 2.10.0 does not send the version and we are about to release version 2.11.0. I think we should refrain from releasing 2.11.0 until we remove version. Does that sound reasonable to you @alex-spies ?

That sounds very reasonable! I expect that ES 8.15 will want to disallow version again, so it'd be better if rally didn't send this; if it's not too much trouble, waiting until elastic/elasticsearch#108919 (review) is merged and backported would be great IMHO.

Also, if we ever have to investigate ES|QL regression in the range of ES commits where version is required, we will have to use specific Rally commit ID (from before this PR), which is not a major problem.

That is great to hear, thank you!

@gareth-ellis gareth-ellis added this to the 2.11.0 milestone May 23, 2024
@gareth-ellis gareth-ellis added the cleanup Linter changes, reformatting, removal of unused code etc. label May 23, 2024
gbanasiak
gbanasiak approved these changes May 23, 2024
View reviewed changes
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 assuming tests start passing once elastic/elasticsearch#108919 is in

@gbanasiak
Copy link
Contributor

elastic/elasticsearch#108919 is merged, so I've initiated CI re-runs.

@gbanasiak
Copy link
Contributor

We will have to refrain from merging until ES change propagates to all serverless environments.

@pquentin
Copy link
Member

buildkite test this please

@gbanasiak gbanasiak merged commit fc8822e into elastic:master May 29, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbanasiak gbanasiak gbanasiak approved these changes

@pquentin pquentin Awaiting requested review from pquentin

@nik9000 nik9000 Awaiting requested review from nik9000

Assignees
No one assigned
Labels
cleanup Linter changes, reformatting, removal of unused code etc.
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@alex-spies @gbanasiak @pquentin @gareth-ellis