Skip to content

Conversation

kiraksi
Copy link
Contributor

@kiraksi kiraksi commented Jan 19, 2024

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • 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)

Fixes #1790 🦕

@product-auto-label product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Jan 19, 2024
@kiraksi kiraksi marked this pull request as ready for review January 19, 2024 23:46
@kiraksi kiraksi requested review from a team as code owners January 19, 2024 23:46
@kiraksi kiraksi requested review from PhongChuong, tswast and chalmerlowe and removed request for PhongChuong January 19, 2024 23:46
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Thanks!

I have some feedback regarding testing. I think it may make more sense to follow the pattern I use here, instead:

def test_query_and_wait_retries_job():
freezegun.freeze_time(auto_tick_seconds=100)
client = mock.create_autospec(Client)
client._call_api.__name__ = "_call_api"
client._call_api.__qualname__ = "Client._call_api"
client._call_api.__annotations__ = {}
client._call_api.__type_params__ = ()
client._call_api.side_effect = (
google.api_core.exceptions.BadGateway("retry me"),
google.api_core.exceptions.InternalServerError("job_retry me"),
google.api_core.exceptions.BadGateway("retry me"),
{
"jobReference": {
"projectId": "response-project",
"jobId": "abc",
"location": "response-location",
},
"jobComplete": True,
"schema": {
"fields": [
{"name": "full_name", "type": "STRING", "mode": "REQUIRED"},
{"name": "age", "type": "INT64", "mode": "NULLABLE"},
],
},
"rows": [
{"f": [{"v": "Whillma Phlyntstone"}, {"v": "27"}]},
{"f": [{"v": "Bhetty Rhubble"}, {"v": "28"}]},
{"f": [{"v": "Phred Phlyntstone"}, {"v": "32"}]},
{"f": [{"v": "Bharney Rhubble"}, {"v": "33"}]},
],
},
)
rows = _job_helpers.query_and_wait(
client,
query="SELECT 1",
location="request-location",
project="request-project",
job_config=None,
page_size=None,
max_results=None,
retry=retries.Retry(
lambda exc: isinstance(exc, google.api_core.exceptions.BadGateway),
multiplier=1.0,
).with_deadline(
200.0
), # Since auto_tick_seconds is 100, we should get at least 1 retry.
job_retry=retries.Retry(
lambda exc: isinstance(exc, google.api_core.exceptions.InternalServerError),
multiplier=1.0,
).with_deadline(600.0),
)
assert len(list(rows)) == 4
# For this code path, where the query has finished immediately, we should
# only be calling the jobs.query API and no other request path.
request_path = "/projects/request-project/queries"
for call in client._call_api.call_args_list:
_, kwargs = call
assert kwargs["method"] == "POST"
assert kwargs["path"] == request_path

It may make sense to also add a test for just the predicate itself:

def test_DEFAULT_JOB_RETRY_predicate():

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 22, 2024
@kiraksi kiraksi requested a review from tswast January 22, 2024 22:56

_, kwargs = calls[2]
assert kwargs["method"] == "POST"
assert kwargs["path"].startswith(response_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting one. Not sure why we'd be doing a POST to this path. That'd indicate that we are doing a jobs insert call, which really shouldn't be happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a todo statement for the fix in #1797

@kiraksi kiraksi requested a review from tswast January 24, 2024 21:28
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Thanks!

@nlenepveu
Copy link

Thanks @kiraksi!

@kiraksi kiraksi deleted the job-rate-limit branch January 24, 2024 23:22

from google.cloud.bigquery.client import Client
from google.cloud.bigquery import _job_helpers
from google.cloud.bigquery.retry import DEFAULT_JOB_RETRY
Copy link
Contributor

Choose a reason for hiding this comment

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

Import modules not classes / methods.

https://google.github.io/styleguide/pyguide.html#22-imports

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. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: client doesn't retry "Job exceeded rate limits" for DDL query jobs that exceed quota for table update operations
3 participants