search

elastic / rally

Macrobenchmarking framework for Elasticsearch
Apache License 2.0
1.95k stars 313 forks source link

Remove esql version #1849

Closed alex-spies closed 4 months ago

alex-spies commented 4 months ago

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 https://github.com/elastic/rally/pull/1841.

This should be merged only after https://github.com/elastic/elasticsearch/pull/108919 is merged to avoid breakage.

gbanasiak commented 4 months ago

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 https://github.com/elastic/elasticsearch/pull/108919 is merged they should pass.

gbanasiak commented 4 months ago

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.

alex-spies commented 4 months ago

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 https://github.com/elastic/elasticsearch/pull/108919#pullrequestreview-2073142605 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!

gbanasiak commented 4 months ago

https://github.com/elastic/elasticsearch/pull/108919 is merged, so I've initiated CI re-runs.

gbanasiak commented 4 months ago

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

pquentin commented 4 months ago

buildkite test this please