-
Notifications
You must be signed in to change notification settings - Fork 133
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
Issue 910 - Added timeout to BigQuery load_job.result() #981
Conversation
parsons/google/google_bigquery.py
Outdated
@@ -464,7 +467,7 @@ def copy_from_gcs( | |||
job_config=job_config, | |||
**load_kwargs, | |||
) | |||
load_job.result() | |||
load_job.result(timeout=MAX_TIMEOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a constant, perhaps this should be a keyword parameter that defaults to 30. Something like:
def get_table_ref(client, table_name, timeout = 30):
That way if 30 isn't enough for my use case, or is too long, I can adapt the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, for sure! I think that is a cleaner implementation, good suggestion
parsons/google/google_bigquery.py
Outdated
@@ -632,7 +639,10 @@ def copy_large_compressed_file_from_gcs( | |||
job_config=job_config, | |||
**load_kwargs, | |||
) | |||
load_job.result() | |||
load_job.result(timeout=MAX_TIMEOUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
Hi, looked into why the Mac build is failing (see here). Github isn't reporting any outages — not too sure what is going on
|
Sometimes this happens with these mac tests :'( I'm rerunning it and hopefully it'll pass. |
Ominous that it's a timeout error 😂 |
Looks like the tests are passing now! What's the status of this PR? Are we waiting on re-review, or on changes? |
I might have missed something, but I think all changes have been addressed. |
Awesome! @Jason94 whenever you get a chance can you re-review? |
Overview
This PR addresses Issue #910. A timeout is added to prevent indefinite hanging.
From the original issue:
Changes
27/1/2024
AddedMAX_TIMEOUT = 30
constantUpdatedload_job.result()
to useMAX_TIMEOUT
31/1/2024 — @Jason94
max_timeout: int = 30
as a keyword argumentload_job.result()
to usemax_timeout
DeadlineExceeded
error handling (docs)