feat: add default timeout for Client.get_job() #1935

Linchin · 2024-05-29T01:49:02Z

When calling QueryJob.result(), QueryJob.reload() is called in the background with an empty timeout value. This leads QueryJob to waiting indefinitely if QueryJob.reload() loses connection.

This PR:

Reroutes QueryJob.reload() to call Client.get_job() instead of calling API directly
Adds a 128s default timeout for Client.get_job()

Fixes #1922 🦕

tswast

Please add some tests where *Job.reload(timeout=None), *Job.done(timeout=None), and *Job.result(timeout=None) disable the timeouts for jobs.get.

google/cloud/bigquery/client.py

google/cloud/bigquery/job/base.py

tswast · 2024-05-30T16:21:42Z

google/cloud/bigquery/job/base.py

-            extra_params["location"] = self.location
-        span_attributes = {"path": self.path}
+        kwargs: Dict[str, Any] = {}
+        if type(timeout) is object:


Let's add some comments explaining why we need to omit the timeout if we get _DEFAULT_VALUE. (Because we want to use the default API-level timeouts but wait indefinitely for the job to finish)

I added explanation why kwargs is used here, but I feel like Queryjob.result() is a better place to explain the default timeout behavior, so I added it in docstring there instead.

tswast · 2024-05-30T16:23:41Z

google/cloud/bigquery/job/base.py

@@ -913,7 +922,7 @@ def _set_future_result(self):
    def done(
        self,
        retry: "retries.Retry" = DEFAULT_RETRY,
-        timeout: Optional[float] = None,
+        timeout: Optional[Union[float, object]] = PollingFuture._DEFAULT_VALUE,


Done isn't waiting for the job to complete, so I wonder if we actually need PollingFuture._DEFAULT_VALUE here?

It seems more analogous to Client.get_job so maybe we can use the same DEFAULT_GET_JOB_TIMEOUT, here? 🤔 We need to double-check that we aren't calling any other API requests like getQueryResults, if so.

done() calls reload(), which in turn calls Client.get_job(). So I added the sentinel values along the path result() -> done() -> reload(). Indeed I realize we don't have to add sentinel for done() or reload(), because it will be passed from result() anyway. As to getQueryResults, I don't think it's called from done() onward (it's used in result()). I think either way (using PollingFuture._DEFAULT_VALUE or DEFAULT_GET_JOB_TIMEOUT) it's effectively the same, so I think either way it's fine. Are there any other factors I'm not aware of?

I'd like to keep our type annotations as narrow as we can. My worry with PollingFuture._DEFAULT_VALUE is it opens it up to object, which technically is anything (string, bytes, anything) in Python.

Indeed, I ended up only using sentinel for result(), and used DEFAULT_GET_JOB_TIMEOUT for subsequent calls.

tswast · 2024-05-30T16:24:18Z

google/cloud/bigquery/job/query.py

+        if type(timeout) is object:
+            timeout = None
+            get_job_timeout = PollingFuture._DEFAULT_VALUE
+        else:
+            get_job_timeout = timeout


Why not do the kwargs trick here too?

I wonder if it's necessary - the purpose here is slightly different from when calling client.get_job(). I'm only preserving the sentinel value for the job.done() call, and use None as default for any other calls.

done, following later comments

tests/unit/test__job_helpers.py

tswast · 2024-05-31T20:07:48Z

google/cloud/bigquery/job/base.py

@@ -801,7 +801,7 @@ def reload(
        self,
        client=None,
        retry: "retries.Retry" = DEFAULT_RETRY,
-        timeout: Optional[float] = None,
+        timeout: Optional[Union[float, object]] = POLLING_DEFAULT_VALUE,


I'd much rather this defaulted to the same timeout as get_job() and anywhere that calls reload(), we make sure we do the check for POLLING_DEFAULT_VALUE before calling reload() to make sure we only pass in float or None or omit the value.

tswast · 2024-05-31T20:08:50Z

google/cloud/bigquery/job/base.py

@@ -913,7 +924,7 @@ def _set_future_result(self):
    def done(
        self,
        retry: "retries.Retry" = DEFAULT_RETRY,
-        timeout: Optional[float] = None,
+        timeout: Optional[Union[float, object]] = POLLING_DEFAULT_VALUE,


Same here. I don't think this needs to handle POLLING_DEFAULT_VALUE since it's not waiting for the job to finish, just making at most 1 API call.

google/cloud/bigquery/job/query.py

tswast · 2024-05-31T20:12:33Z

google/cloud/bigquery/job/query.py

+        # timeout to QueryJob.done(), and use None for the other timeouts.
+        if type(timeout) is object:
+            timeout = None
+            get_job_timeout = POLLING_DEFAULT_VALUE


Why not set to DEFAULT_GET_JOB_TIMEOUT or do the kwargs trick to omit it?

Co-authored-by: Tim Sweña (Swast) <[email protected]>

tswast

Love it! Just a couple of typos to fix please before merging

google/cloud/bigquery/job/base.py

feat: add default timeout for Client.get_job()

83c63e2

Linchin assigned tswast May 29, 2024

Linchin requested review from a team as code owners May 29, 2024 01:49

Linchin requested a review from leahecole May 29, 2024 01:49

product-auto-label bot added size: l Pull request size is large. api: bigquery Issues related to the googleapis/python-bigquery API. labels May 29, 2024

Linchin requested review from tswast and removed request for leahecole May 29, 2024 01:50

Linchin added 8 commits May 28, 2024 18:50

Merge branch 'main' into timeout

3b0b09b

change timeout type detection

4231ba1

lint

24baa1c

fix unit test and coverage

358b32e

Merge branch 'main' into timeout

73ad570

add type hint

66fde13

Merge branch 'main' into timeout

2a23cda

fix type hint

1f1abc3

tswast requested changes May 30, 2024

View reviewed changes

Linchin added 9 commits May 30, 2024 13:37

Merge branch 'main' into timeout

5728272

change import style and add comments

c06ca38

remove sentinel value in client

1d58589

type hint

9bfe28c

typo

97844d1

add sentinel for query_and_wait()

0b1220e

add unit tests

1a5fa7c

Merge branch 'main' into timeout

ed5f6cb

fix unit test

4d77e36

tswast reviewed May 31, 2024

View reviewed changes

Linchin and others added 5 commits May 31, 2024 13:23

Update google/cloud/bigquery/job/query.py

bc398ee

Co-authored-by: Tim Sweña (Swast) <[email protected]>

Update google/cloud/bigquery/job/query.py

ef7b71e

Co-authored-by: Tim Sweña (Swast) <[email protected]>

address comments

56893c0

typo

4987ccd

type hint

253e274

tswast approved these changes May 31, 2024

View reviewed changes

google/cloud/bigquery/job/base.py Outdated Show resolved Hide resolved

google/cloud/bigquery/job/base.py Outdated Show resolved Hide resolved

typos

bd25eea

Linchin merged commit 9fbad76 into googleapis:main May 31, 2024
18 checks passed

release-please bot mentioned this pull request May 31, 2024

chore(main): release 3.24.0 #1928

Merged

This was referenced Jun 11, 2024

June 10, 2024 kitta65/bq-extension-vscode#422

Closed

June 10, 2024 kitta65/prettier-plugin-bq#404

Closed

June 10, 2024 kitta65/bq2cst#378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add default timeout for Client.get_job() #1935

feat: add default timeout for Client.get_job() #1935

Linchin commented May 29, 2024 •

edited

Loading

tswast left a comment

tswast May 30, 2024

Linchin May 30, 2024

tswast May 30, 2024

Linchin May 30, 2024 •

edited

Loading

tswast May 31, 2024

Linchin May 31, 2024

tswast May 30, 2024

Linchin May 30, 2024

Linchin May 31, 2024

tswast May 31, 2024

Linchin May 31, 2024

tswast May 31, 2024

Linchin May 31, 2024

tswast May 31, 2024

Linchin May 31, 2024

tswast left a comment

feat: add default timeout for Client.get_job() #1935

feat: add default timeout for Client.get_job() #1935

Conversation

Linchin commented May 29, 2024 • edited Loading

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Linchin May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

Linchin commented May 29, 2024 •

edited

Loading

Linchin May 30, 2024 •

edited

Loading