NotificationsYou must be signed in to change notification settings
Fork321
Star786

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Linchin merged 24 commits intogoogleapis:mainfromLinchin:timeout

May 31, 2024

Merged

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

Linchin merged 24 commits intogoogleapis:mainfromLinchin:timeout

May 31, 2024

Conversation

Copy link

Contributor

Linchin commentedMay 29, 2024•
edited
Loading

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

This PR:

ReroutesQueryJob.reload() to callClient.get_job() instead of calling API directly
Adds a 128s default timeout forClient.get_job()

Fixes#1922 🦕

feat: add default timeout for Client.get_job()

83c63e2

Linchin assignedtswast

May 29, 2024

Linchin requested review froma team ascode owners

May 29, 2024 01:49

Linchin requested a review fromleahecole

May 29, 2024 01:49

product-auto-labelbot added size: l

Pull request size is large.

api: bigqueryIssues related to the googleapis/python-bigquery API. labels

May 29, 2024

Linchin requested review fromtswast and removed request forleahecole

May 29, 2024 01:50

Linchin added8 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

Copy link

Contributor

tswast left a comment

Choose a reason for hiding this comment

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

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

google/cloud/bigquery/client.py OutdatedShow resolvedHide resolved

google/cloud/bigquery/job/base.py OutdatedShow resolvedHide resolved

google/cloud/bigquery/job/base.py Outdated

		extra_params["location"]=self.location
		span_attributes= {"path":self.path}
		kwargs:Dict[str,Any]= {}
		iftype(timeout)isobject:

Copy link

Contributor

Choose a reason for hiding this comment

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

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)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

google/cloud/bigquery/job/base.py Outdated

		self,
		retry:"retries.Retry"=DEFAULT_RETRY,
		timeout:Optional[float]=None,
		timeout:Optional[Union[float,object]]=PollingFuture._DEFAULT_VALUE,

Copy link

Contributor

Choose a reason for hiding this comment

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

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

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

Copy link

ContributorAuthor

LinchinMay 30, 2024•
edited
Loading

Choose a reason for hiding this comment

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

done() callsreload(), which in turn callsClient.get_job(). So I added the sentinel values along the pathresult() ->done() ->reload(). Indeed I realize we don't have to add sentinel fordone() orreload(), because it will be passed fromresult() anyway. As to getQueryResults, I don't think it's called fromdone() onward (it's used inresult()). I think either way (usingPollingFuture._DEFAULT_VALUE orDEFAULT_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?

Copy link

Contributor

tswastMay 31, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

LinchinMay 31, 2024

Choose a reason for hiding this comment

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

Indeed, I ended up only using sentinel forresult(), and usedDEFAULT_GET_JOB_TIMEOUT for subsequent calls.

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

Comment on lines 1518 to 1522

		iftype(timeout)isobject:
		timeout=None
		get_job_timeout=PollingFuture._DEFAULT_VALUE
		else:
		get_job_timeout=timeout

Copy link

Contributor

tswastMay 30, 2024

Choose a reason for hiding this comment

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

Why not do the kwargs trick here too?

Copy link

ContributorAuthor

LinchinMay 30, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

LinchinMay 31, 2024

Choose a reason for hiding this comment

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

done, following later comments

tests/unit/test__job_helpers.py OutdatedShow resolvedHide resolved

Linchin added9 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

google/cloud/bigquery/job/base.py Outdated

		client=None,
		retry:"retries.Retry"=DEFAULT_RETRY,
		timeout:Optional[float]=None,
		timeout:Optional[Union[float,object]]=POLLING_DEFAULT_VALUE,

Copy link

Contributor

tswastMay 31, 2024

Choose a reason for hiding this comment

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

I'd much rather this defaulted to the same timeout asget_job() and anywhere that callsreload(), we make sure we do the check forPOLLING_DEFAULT_VALUE before callingreload() to make sure we only pass infloat orNone or omit the value.

Copy link

ContributorAuthor

LinchinMay 31, 2024

Choose a reason for hiding this comment

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

done

tswast reviewed

May 31, 2024

View reviewed changes

google/cloud/bigquery/job/base.py Outdated

		self,
		retry:"retries.Retry"=DEFAULT_RETRY,
		timeout:Optional[float]=None,
		timeout:Optional[Union[float,object]]=POLLING_DEFAULT_VALUE,

Copy link

Contributor

tswastMay 31, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

LinchinMay 31, 2024

Choose a reason for hiding this comment

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

done

tswast reviewed

May 31, 2024

View reviewed changes

google/cloud/bigquery/job/query.py OutdatedShow resolvedHide resolved

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

		# timeout to QueryJob.done(), and use None for the other timeouts.
		iftype(timeout)isobject:
		timeout=None
		get_job_timeout=POLLING_DEFAULT_VALUE

Copy link

Contributor

tswastMay 31, 2024

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

LinchinMay 31, 2024

Choose a reason for hiding this comment

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

done

Linchinand others added5 commits

May 31, 2024 13:23

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

bc398ee

Co-authored-by: Tim Sweña (Swast) <swast@google.com>

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

ef7b71e

Co-authored-by: Tim Sweña (Swast) <swast@google.com>

address comments

56893c0

typo

4987ccd

type hint

253e274

tswast approved these changes

May 31, 2024

View reviewed changes

Copy link

Contributor

tswast left a comment

Choose a reason for hiding this comment

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

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

google/cloud/bigquery/job/base.py OutdatedShow resolvedHide resolved

typos

bd25eea

Linchin merged commit9fbad76 intogoogleapis:main

May 31, 2024

release-pleasebot mentioned this pull request

May 31, 2024

chore(main): release 3.24.0#1928

Merged

This was referencedJun 11, 2024

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

Closed

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

Closed

June 10, 2024kitta65/bq2cst#378

Closed

Labels

api: bigquery

Issues related to the googleapis/python-bigquery API.

size: l

Pull request size is large.

Movatterモバイル変換

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

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

Uh oh!

Conversation

Linchin commentedMay 29, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinchinMay 30, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Linchin commentedMay 29, 2024•
edited
Loading

LinchinMay 30, 2024•
edited
Loading