search

elastic / rally

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

Detect serverless operator status automatically #1768

Closed pquentin closed 1 year ago

pquentin commented 1 year ago

This can be tested like https://github.com/elastic/rally/pull/1760, but you no longer need to explicitly set the operator status in rally.ini, instead it should be detected automatically.

pquentin commented 1 year ago

I get the sense that it might be better to split out the retrieval of the versions, operator status, and REST API health check into discrete functions, but I don't think it's worth blocking this PR.

Well this is definitely quite ugly. I want to use a single Elasticsearch client here but did not want to affect the stateful uses of this function too much. This is the best compromise I found. However, if we think the logic of this function is fine, then I'm not convinced separating it into other functions would have helped that much. The logic is complicated but 20 lines is not that much in my opinion, even though Uncle Bob would disagree.

b-deam commented 1 year ago

I get the sense that it might be better to split out the retrieval of the versions, operator status, and REST API health check into discrete functions, but I don't think it's worth blocking this PR.

Well this is definitely quite ugly. I want to use a single Elasticsearch client here but did not want to affect the stateful uses of this function too much. This is the best compromise I found. However, if we think the logic of this function is fine, then I'm not convinced separating it into other functions would have helped that much. The logic is complicated but 20 lines is not that much in my opinion, even though Uncle Bob would disagree.

I agree, no need for any changes. :shipit: