search

elastic / rally

Macrobenchmarking framework for Elasticsearch
Apache License 2.0
1.91k stars 314 forks source link

Send version with esql #1841

Closed nik9000 closed 2 months ago

nik9000 commented 2 months ago

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.

pquentin commented 2 months ago

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 commented 2 months ago

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, https://github.com/elastic/elasticsearch/pull/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 commented 2 months ago

https://github.com/elastic/elasticsearch/pull/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 commented 2 months ago

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 https://github.com/elastic/elasticsearch/pull/107438; but that'd be adding more loopholes into the API for pre-GA features.

alex-spies commented 2 months ago

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 commented 2 months ago

run CI / unit macOS 3.8 (pull_request)

gbanasiak commented 2 months ago

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

nik9000 commented 2 months ago

Merged!

nik9000 commented 2 months ago

sorry for the delay