Add new wait-for-current-snapshots-create operation#1542
Add new wait-for-current-snapshots-create operation#1542dliappis merged 9 commits intoelastic:masterfrom
Conversation
This commit adds a new runner `WaitForCurrentSnapshotsCreate`, with the corresponding operation name `wait-for-current-snapshots-create` that waits until all current snapshots for a given repository have completed. Closes elastic#1537
dliappis
added
enhancement
pquentin
left a comment
pquentin
left a comment
There was a problem hiding this comment.
Thanks! LGTM. Left two nits.
esrally/driver/runner.py
Outdated
| headers["Content-Type"] = "application/json" | ||
|
|
||
| # significantly reduce response size when lots of snapshots have been taken | ||
| # https://github.com/elastic/elasticsearch/pull/86269 |
There was a problem hiding this comment.
Not sure the link to the pull request helps that much now that 8.3 is out? Maybe we can put in the TODO instead. So when the TODO gets removed, this gets removed too.
There was a problem hiding this comment.
The reasoning behind putting the PR link here is to clarify that this feature (optionally excluding the large number of indices from the response) is only available from ES 8.3.0 onwards.
The TODO, later on, is about the support in elasticsearch-py since currently the index_names parameter in the version that we use is not supported.
I have improved and moved the comment to the beginning of the conditional in e482d72 to make this clearer. The TODO remained as is.
esrally/driver/runner.py
Outdated
| if int(response.get("total")) > 0: | ||
| await asyncio.sleep(wait_period) | ||
| continue | ||
| break |
There was a problem hiding this comment.
Here's a potential simplification:
| if int(response.get("total")) > 0: | |
| await asyncio.sleep(wait_period) | |
| continue | |
| break | |
| if int(response.get("total")) == 0: | |
| break | |
| await asyncio.sleep(wait_period) |
Also this logic is untested, should we in tests first have a response with total > 0?
There was a problem hiding this comment.
Thanks added the simplification in 87b7040.
Also this logic is untested, should we in tests first have a response with total > 0?
Not sure I understood this comment, we do test with total > 0 e.g. in
rally/tests/driver/runner_test.py
Line 4029 in ac80c61
rally/tests/driver/runner_test.py
Line 4047 in ac80c61
michaelbaamonde
left a comment
michaelbaamonde
left a comment
There was a problem hiding this comment.
I left a minor suggestion, but otherwise LGTM.
|
Latest commits also LGTM. |
|
Thanks everyone for the great feedback! |
This commit adds a new challenge for many-shards that benchmarks ES performance while taking several (specified by the track parameter `snapshot_counts`) snapshots. The intention is to initially use it for tracking regressions by visualizing the `service_time` of the `wait-for-snapshots` task. This PR depends uses the Rally operation `wait-current-snapshots-create` operation (introduced in elastic/rally#1542).
4 participants
This commit adds a new runner
WaitForCurrentSnapshotsCreate, with thecorresponding operation name
wait-for-current-snapshots-createthatwaits until all current snapshots for a given repository have completed.
Closes #1537