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.

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

Add new wait-for-current-snapshots-create operation #1542

Merged
merged 9 commits into from
Aug 9, 2022

Conversation

dliappis
Copy link
Contributor

@dliappis dliappis commented Jul 8, 2022

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 #1537

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 dliappis added enhancement Improves the status quo :Track Management New operations, changes in the track format, track download changes and the like labels Jul 8, 2022
@dliappis dliappis self-assigned this Jul 8, 2022
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM. Left two nits.

docs/track.rst Outdated Show resolved Hide resolved
headers["Content-Type"] = "application/json"

# significantly reduce response size when lots of snapshots have been taken
# https://github.com/elastic/elasticsearch/pull/86269
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 2077 to 2080
if int(response.get("total")) > 0:
await asyncio.sleep(wait_period)
continue
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a potential simplification:

Suggested change
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"total": 4,
and ensure we wait the right amount of times in
assert es.snapshot.get.await_count == 2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry I missed this.

Copy link
Contributor

@michaelbaamonde michaelbaamonde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a minor suggestion, but otherwise LGTM.

esrally/driver/runner.py Outdated Show resolved Hide resolved
esrally/driver/runner.py Show resolved Hide resolved
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@dliappis dliappis requested a review from pquentin August 8, 2022 07:44
@michaelbaamonde
Copy link
Contributor

Latest commits also LGTM.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

@dliappis dliappis merged commit 6edfa43 into elastic:master Aug 9, 2022
@dliappis
Copy link
Contributor Author

dliappis commented Aug 9, 2022

Thanks everyone for the great feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :Track Management New operations, changes in the track format, track download changes and the like
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance wait-for-snapshot-create operation to wait for all current snapshots
4 participants