Remove num_retries parameter from load_table_from_*() methods

[plamut]

When dealing with missing timeout for resumable upload requests, I noticed several comments that the num_retries parameter is deprecated and is planned to be removed. If this is still the case, we can use the opportunity and remove it in v3.

Examples:

• In _do_resumable_upload()
• In _initiate_resumable_upload()
• In _do_multipart_upload()

[plamut]

@tswast Those comments were added at least two years ago, and we haven't actually been issuing any deprecation warnings.

Are the comments still valid?

[tswast]

Looks like they were there even 4 years ago. googleapis/google-cloud-python#4136

Edit: even before that: googleapis/google-cloud-python#3555

Edit: I think we actually inherited it from GCS: googleapis/google-cloud-python#3378

Edit: This was the PR with original deprecation/removal: googleapis/google-cloud-python#3362

[tswast]

I imagine the intent was to deprecate it in favor of a retry object? Doesn't look like we actually did that, though.

Maybe in v3 we add the retry logic -- potentially similar to job_retry in query, seeing as this involves starting the stream from the beginning, which is slightly different from the per-request retry object?

If we get that working, then I think in v3 we just start warning for num_retries rather than remove it, seeing as we haven't actually had a replacement available.

[tswast]

Looks like the intent was to replace it with a https://googleapis.dev/python/google-resumable-media/latest/resumable_media/common.html?highlight=retrystrategy#google.resumable_media.common.RetryStrategy, actually?

[tswast]

Looks like google-cloud-storage has a converter from Retry -> RetryStrategy.

https://github.com/googleapis/python-storage/blob/af9c9dc83d8582167b74105167af17c9809455de/google/cloud/storage/blob.py#L1912

We might want to do something similar.

Edit: See where this was added in googleapis/python-storage#447

[plamut]

I imagine the intent was to deprecate it in favor of a retry object? Doesn't look like we actually did that, though.

That's quite possible. A retry strategy is much more expressive than a plain timeout value.

I agree that issuing deprecation warnings first would be a more user-friendly approach. I wouldn't necessarily include this in the 3.0 release, though, maybe just the deprecation warning? IMHO there's more value in finalizing the first v3 release by the end of the year, and only include this feature in 3.0.0 if the capacity allows for it, i.e. if it doesn't cause delays.

[tswast]

Agreed that it's a low priority and that we'd probably only include the deprecation warning + retry feature in v3. A stable v3 is definitely more important, and this could happen post-v3 release.

[chalmerlowe]

Gonna mark this as a P3 priority based on the above conversation.

[chalmerlowe]

This is still on the back burner. If we intend to do this, the first steps are:

• identify where the num_retries parameter is still being used AND needs to be removed
• add deprecation warnings to the code relative to those uses