search

googleapis / python-storage

Apache License 2.0
443 stars 152 forks source link

Resumable media retry appears to treat `deadline` differently than google.api.core.Retry #1361

Open samlevy3 opened 2 weeks ago

samlevy3 commented 2 weeks ago

Environment details

Description

For resumable media calls in cloud storage (ex: download_as_string), the provided google.api.core.Retry is converted to a google.resumable_media.RetryStrategy. However, the meaning of the provided deadline appears to differ between these two classes.

For google.api.core.Retry, deadline seems to be defined as "the maximum amount of time a function can block"

However, for google.resumable_media.RetryStrategy, max_cumulative_retry (what deadline corresponds to) is defined as "The maximum total amount of time to sleep during the retry process."

There appears to be a mismatch between the definitions though. One refers to the total time the function blocks while the other refers to the cumulative time spent sleeping between retries.

If this is indeed a mismatch is it possible to update the documentation of resumable media operations to clarify that they have a different retry strategy?

andrewsg commented 2 weeks ago

There is a mismatch. We intend to unify the two retry strategies either during the next major version release or in a feature release soon after that. In the meantime we'll see if we can make tweaks to the two strategies to bring them closer together.

samlevy3 commented 2 weeks ago

Thanks for the reply @andrewsg! Do you have information about what unification will look like? Will the google.resumable_media.RetryStrategy be changed to match the google.api.core.Retry?

andrewsg commented 2 weeks ago

I would like it if it would turn out like that, but the resumable media retry system is much more complicated than the standard retry system, as it counts bytes written to stream and resumes from there whenever possible, so given that it's not fully designed yet I can't guarantee the outcome. Unification might look like standardizing on google.api.core.Retry, or it might look like something purpose-built that is backwards compatible with those Retry objects.

samlevy3 commented 1 week ago

Understood. I will keep an eye on it and adjust if necessary based on the outcome! Thanks for the clarification!