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

Provided timeout should be applied to all resumable upload requests #1067

Closed
amo12937 opened this issue Nov 17, 2021 · 2 comments · Fixed by #1070
Closed

Provided timeout should be applied to all resumable upload requests #1067

amo12937 opened this issue Nov 17, 2021 · 2 comments · Fixed by #1070
Assignees
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@amo12937
Copy link

I investigated ReadTimeout error that is thrown in Client.load_table_from_json.

The error occurred at ResumableUpload.transmit_next_chunk in _do_resumable_upload in client.py.
Lookig at the code, timeout argument is passed to _initiate_resumable_upload but not to transmit_next_chunk, and the default timeout (which is 60 sec) will be used.
Is this intentional, or does anyone plan to fix it?
Thank you.

@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Nov 17, 2021
@plamut plamut added the type: question Request for information or clarification. Not an issue. label Nov 17, 2021
@plamut
Copy link
Contributor

plamut commented Nov 17, 2021

Hm... the line containing the transmit_next_chunk() code was written more than 4 years ago, while the support for customizable timeouts in resumable media's ResumableUpload class have only been added ~17 months ago. For some reason this hasn't been leveraged in the BigQuery client (yet?).

The timeouts were added to several BigQuery methods 13 months ago in #327, including the change to _initiate_resumable_upload(), but the upload.transmit_next_chunk(...) call was left intact.

@tswast Do you maybe remember if there was a specific reason for that?
I was on a short sabbatical back then, so I can't say, but there's nothing on the PR either, thus it seems like an oversight to me.

@tswast
Copy link
Contributor

tswast commented Nov 17, 2021

I don't recall a specific reason. Agreed that it's most likely just an oversight

@plamut plamut self-assigned this Nov 18, 2021
@plamut plamut changed the title Why does not _do_resumable_upload method pass timeout argument to transmit_next_chunk method? Provided timeout should be applied to all resumable upload requests Nov 18, 2021
@plamut plamut added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants