Skip to content

Handle BigQuery 503 error#1938

Closed
krmettu wants to merge 4 commits intoapache:masterfrom
krmettu:master
Closed

Handle BigQuery 503 error#1938
krmettu wants to merge 4 commits intoapache:masterfrom
krmettu:master

Conversation

@krmettu
Copy link

@krmettu krmettu commented Dec 9, 2016

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

https://issues.apache.org/jira/browse/AIRFLOW-667

We have encountered this issue few times in our production environment. I added this fix based on google's documentation.

https://cloud.google.com/bigquery/troubleshooting-errors

"This error returns when there is a temporary server failure such as a network connection problem or a server overload. In general, wait a few seconds and try again."

Testing Done:

Tests are done connecting to BQ tables using the PR code.
Subject - Handle BigQuery 503 error
added retry feature if BigQuery error is 500 or 503
set the retry time for 5 seconds

Krishnaveni Mettu added 2 commits December 8, 2016 14:38
added retry feature if BigQuery error is 500 or 503
set the retry time for 5 seconds
@codecov-io
Copy link

codecov-io commented Dec 9, 2016

Current coverage is 66.54% (diff: 100%)

Merging #1938 into master will decrease coverage by 0.01%

@@             master      #1938   diff @@
==========================================
  Files           135        135          
  Lines         10181      10189     +8   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6776       6780     +4   
- Misses         3405       3409     +4   
  Partials          0          0          

Powered by Codecov. Last update 2306892...542fc50

@criccomini
Copy link
Contributor

When testing, did you actually experience a 500 error, and the code worked properly?

Copy link
Contributor

@criccomini criccomini left a comment

Choose a reason for hiding this comment

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

Looks good to me, structurally. Few nits and style issues--left comments.

@@ -473,18 +473,29 @@ def run_with_configuration(self, configuration):
job = jobs.get(projectId=self.project_id, jobId=job_id).execute()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need this if you are running it again at the beginning of the try?

Copy link
Author

Choose a reason for hiding this comment

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

@criccomini you are right. I don't need it. copy/paste mistake from out internal git. Thanks for pointing out. I updated the code following your comments.
Please let me know if there are more comments. Thanks so much.

job['status']['errorResult'], job
)
)
job_status_flag = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: rename to keep_polling_job


except HttpError, err:
if err.code in [500, 503]:
logging.info('%s: Retryable error, waiting for job to complete: %s',err.code, job_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add space before err.code

if err.code in [500, 503]:
logging.info('%s: Retryable error, waiting for job to complete: %s',err.code, job_id)
time.sleep(5)
else:raise Exception(
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline for raise

Krishnaveni Mettu added 2 commits December 13, 2016 11:10
@asfgit asfgit closed this in cac1330 Dec 14, 2016
@criccomini
Copy link
Contributor

Thanks!

alekstorm pushed a commit to alekstorm/incubator-airflow that referenced this pull request Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants