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(bigquery): add timeout parameter to client's and job's public methods #10002

Merged
merged 7 commits into from
Dec 26, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Dec 20, 2019

Closes #9987.

This PR adds a timeout parameter to the public methods of client and job classes.

TODO

  • Adjust tests to verify that a timeout passed to a method is indeed forwarded to the underlying transport.
  • Adjust the logic in multi-request methods such as result() - timeout should be split among all requests, as opposed to using the timeout for each individual request.
    • Add tests for this logic.
  • Open a separate issue for adding a timeout to load_table_from_*() methods that depend on file upload classes from google-resumable-media - the latter dependency does not support passing an explicit timeout argument to the underlying transport.
    Done - #10005.

PR checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary) - will be covered in #9988.

This is for internal consistency with other methods such as reload().
AN exception is the Client.load_table_from_file() method, and the
methods that depend on it, because adding a timeout requires changes
in the google-resumable-media dependency.
@plamut plamut added the api: bigquery Issues related to the BigQuery API. label Dec 20, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 20, 2019
@plamut plamut requested a review from tswast December 20, 2019 16:08
@plamut
Copy link
Contributor Author

plamut commented Dec 20, 2019

@tswast This PR can be reviewed now.

I still have to add a few final tests for handling the timeout in multi-request methods. I might not be able to finish them today, but that shouldn't be a review blocker (will complete on Monday).

Edit: The tests failure is because the new test dependency freezegun cannot be imported, I did not commit the changes to noxfile yet.

If a method makes multiple requests and is given a timeout, that
timeout should represent the total allowed time for all requests
combined.
@plamut plamut marked this pull request as ready for review December 23, 2019 13:50
@plamut plamut requested a review from a team December 23, 2019 13:50
@tswast
Copy link
Contributor

tswast commented Dec 23, 2019

flake8 tests
tests/unit/test_client.py:5381:76: E231 missing whitespace after ','
Command flake8 tests failed with exit code 1
Session lint failed.

Run black?

@plamut plamut requested a review from tswast December 26, 2019 20:28
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 BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: Add timeout parameter to all Client and Job methods where applicable
3 participants