-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: Implemented BigQueryRetryAlgorithm to retry on the basis of the configured re-triable error messages #1426
feat: Implemented BigQueryRetryAlgorithm to retry on the basis of the configured re-triable error messages #1426
Conversation
…va-bigquery into bigquery-bugfix-1250
…ased on the error message
…ption ex) method to handle BigQueryRetryHelperException
@@ -1362,7 +1366,7 @@ private static QueryResponse getQueryResults( | |||
: jobId.getLocation()); | |||
try { | |||
GetQueryResultsResponse results = | |||
runWithRetries( | |||
BigQueryRetryHelper.runWithRetries( |
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.
Can we add this also to TableResult query(...)
methods? That should also allow you to verify idempotency using requestId
for jobs.query API.
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.
Sure, I have added BigQueryRetryHelper.runWithRetries
for TableResult query(...)
and have implemented testcase testFastQueryRateLimitIdempotency
to test the idempotency
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.
Okay great but I think we need it for the jobs.insert
endpoint too (line 1269). There are 2 query paths here. The one you've updated is the "fast" query path (jobs.query).
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.
Sure, I have implemented BigQueryRetryHelper.runWithRetries
on QueryResponse waitForQueryResults
method, which is used by TableResult getQueryResults
method
…ency of the BigQueryRetryHelper.runWithRetries for TableResult.query(...)
@@ -237,6 +237,10 @@ | |||
} | |||
|
|||
private final BigQueryRpc bigQueryRpc; | |||
private static final BigQueryRetryConfig DEFAULT_RATE_LIMIT_EXCEEDED_RETRY_CONFIG = |
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.
Should we just name this DEFAULT_RETRY_CONFIG
? It currently has ratelimitexceeded and we can potentially add more in the future.
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.
sure, updated it
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQueryErrorMessages.java
Show resolved
Hide resolved
@@ -237,6 +237,10 @@ | |||
} | |||
|
|||
private final BigQueryRpc bigQueryRpc; | |||
private static final BigQueryRetryConfig DEFAULT_RATE_LIMIT_EXCEEDED_RETRY_CONFIG = | |||
BigQueryRetryConfig.newBuilder() | |||
.retryOnMessage(BigQueryErrorMessages.RATE_LIMIT_EXCEEDED_MSG) |
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.
I find this a little extraneous -- why do we need to specify every message we need to retry on? If we build a config, I imagine we just retry on all the specified error messages in the config.
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.
Sure Stephanie. As discussed, I have tried to create it on the lines of com.google.cloud.ExceptionHandler
and this may give us additional flexibility to configure hooks like retryOnStatus
or retryOnReason
later as necessary. Happy to refactor it as needed.
@@ -1362,7 +1366,7 @@ private static QueryResponse getQueryResults( | |||
: jobId.getLocation()); | |||
try { | |||
GetQueryResultsResponse results = | |||
runWithRetries( | |||
BigQueryRetryHelper.runWithRetries( |
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.
Okay great but I think we need it for the jobs.insert
endpoint too (line 1269). There are 2 query paths here. The one you've updated is the "fast" query path (jobs.query).
…itForQueryResults` method, which is used by `TableResult getQueryResults` method
…ponse waitForQueryResults` method, which is used by `TableResult getQueryResults` method" This reverts commit 84a3418.
…ETRY_CONFIG" This reverts commit 22b1706.
…ETRY_CONFIG" This reverts commit 2d21e11.
…ForQueryResults` method, which is used by `TableResult getQueryResults` method
@prash-mi I tried running the ITs a couple times and it continues to fail here:
Could you try running locally to see this happens? |
Implemented
com.google.cloud.bigquery.BigQueryRetryAlgorithm
which usescom.google.cloud.bigquery.BigQueryRetryConfig
to retry on the basis of the configured re-triable error messages.Modified
BigQueryImpl.getQueryResults
to consumeBigQueryRetryHelper.runWithRetries
to retry usingBigQueryRetryAlgorithm
Fixes #1250 ☕️
By default we retry on the following error: