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

feat: allow RetryStrategy to be configured with a custom initial wait and multiplier #216

Merged
merged 4 commits into from May 18, 2021

Conversation

andrewsg
Copy link
Contributor

@andrewsg andrewsg commented May 8, 2021

This is a backwards-compatible change to allow the python-storage client to configure retries in media calls similarly to how it configures retries in API calls.

@andrewsg andrewsg requested review from tritone and cojenco May 8, 2021 06:15
@andrewsg andrewsg requested a review from a team as a code owner May 8, 2021 06:15
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/google-resumable-media-python API. label May 8, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 8, 2021
google/resumable_media/_helpers.py Show resolved Hide resolved
google/resumable_media/common.py Show resolved Hide resolved
@andrewsg
Copy link
Contributor Author

@tseaver thanks, PTAL

Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! @frankyn FYI as well.

@@ -195,6 +203,41 @@ def test_success_with_retry(self, randint_mock, sleep_mock):
sleep_mock.assert_any_call(2.625)
sleep_mock.assert_any_call(4.375)

@mock.patch(u"time.sleep")
@mock.patch(u"random.randint")
def test_success_with_retry_custom_delay(self, randint_mock, sleep_mock):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test!

base_wait, wait_time = calculate_retry_wait(base_wait, retry_strategy.max_sleep)
base_wait, wait_time = calculate_retry_wait(
base_wait, retry_strategy.max_sleep, retry_strategy.multiplier
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of surprising that this is all calculated in the library rather than using a helper (in Go we use gax) but obviously nothing to address here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strong agree. We have such a helper for python-storage but because the transport layer is different I couldn't simply replace the implementation here with that helper. In the future I'd like to do a bit more work to bring resumable media into the fold in terms of unifying its behavior with python-storage and BQ.

@tritone tritone requested a review from frankyn May 18, 2021 18:52
@andrewsg andrewsg merged commit 579a54b into master May 18, 2021
@andrewsg andrewsg deleted the retry-unification branch May 18, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/google-resumable-media-python API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants