-
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: create Job retry for rate limit exceeded with status code 200 #1744
Conversation
BigQueryException createException; | ||
// NOTE(pongad): This double-try structure is admittedly odd. | ||
// translateAndThrow itself throws, and pretends to return an exception only | ||
// so users can pretend to throw. | ||
// This makes it difficult to translate without throwing. | ||
// Fixing this entails some work on BaseServiceException.translate. | ||
// Since that affects a bunch of APIs, we should fix this as a separate change. | ||
final JobInfo jobInfoCopy = jobInfo; |
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.
Please add comment to explain the motivation for doing this.
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.
Actually not needed, removed this variable
new Callable<com.google.api.services.bigquery.model.Job>() { | ||
@Override | ||
public com.google.api.services.bigquery.model.Job call() { | ||
return bigQueryRpc.create(jobPb, optionsMap); | ||
if(idRandom){ | ||
// generate new random job |
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.
Maybe we can expand this comment to say "re-generate a new random job with the same jobInfo when jobId is not provided by the user"
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.
expanded comment as recommended
BigQueryException createException; | ||
// NOTE(pongad): This double-try structure is admittedly odd. | ||
// translateAndThrow itself throws, and pretends to return an exception only | ||
// so users can pretend to throw. | ||
// This makes it difficult to translate without throwing. | ||
// Fixing this entails some work on BaseServiceException.translate. | ||
// Since that affects a bunch of APIs, we should fix this as a separate change. | ||
final JobInfo jobInfoCopy = jobInfo; | ||
final JobId[] finalJobId = new JobId[1]; |
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.
Does this have to be an array? Can we just use jobId
? since we are only storing a single, final jobId?
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.
Tried several approaches and using an array is the only viable option i found. My understanding is that the variable needs to be a final in order to be accessed from within the inner class (BigQueryRetryHelper). If it's a final it can not be modified, therefore the new jobId values cannot be assigned to it
I would really appreciate any suggestions on how to avoid using an array here.
return bigQueryRpc.create(jobPb, optionsMap); | ||
if(idRandom){ | ||
// generate new random job | ||
JobInfo newJobInfo = jobInfoCopy.toBuilder().setJobId(idProvider.get()).build(); |
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.
Maybe rename this variable to recreatedJobInfo
since everything but JobId is the same for this Job.
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.
changed variable name as recommended
e.g. status code 200 with a rate limit exceeded for job create | ||
in these cases there is now previousThrowable so we need to check previousResponse | ||
*/ | ||
errorDesc = previousResponse.toString(); |
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.
Please add a mock test in BigQueryImplTest
to verify the correctness of the behavior; i.e. retrying with status code 200 on rateLimitExceeded error message.
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.
added mock test to BigQueryImplTest
test is called testCreateJobFailureShouldRetry
🤖 I have created a release \*beep\* \*boop\* --- ## [2.6.0](https://www.github.com/googleapis/java-bigquery/compare/v2.5.1...v2.6.0) (2021-12-27) ### Features * create Job retry for rate limit exceeded with status code 200 ([#1744](https://www.github.com/googleapis/java-bigquery/issues/1744)) ([97a61dc](https://www.github.com/googleapis/java-bigquery/commit/97a61dc90fb701986a51a12c9c83b7138894307a)) ### Bug Fixes * **java:** add -ntp flag to native image testing command ([#1299](https://www.github.com/googleapis/java-bigquery/issues/1299)) ([#1738](https://www.github.com/googleapis/java-bigquery/issues/1738)) ([585875e](https://www.github.com/googleapis/java-bigquery/commit/585875e776e17660c58f9f8fe8385f13833bca57)) ### Documentation * rename alter materialized view to update ([#1754](https://www.github.com/googleapis/java-bigquery/issues/1754)) ([0b7d911](https://www.github.com/googleapis/java-bigquery/commit/0b7d91135222505f0eb01e8b40095156a073b62e)) * **samples:** update UpdateTableExpirationIT to fix failing IT. ([#1753](https://www.github.com/googleapis/java-bigquery/issues/1753)) ([a62a9f4](https://www.github.com/googleapis/java-bigquery/commit/a62a9f4fdda465b8c9e2f67f111d1b1b4a067903)) ### Dependencies * update dependency com.google.apis:google-api-services-bigquery to v2-rev20211129-1.32.1 ([#1737](https://www.github.com/googleapis/java-bigquery/issues/1737)) ([776ff10](https://www.github.com/googleapis/java-bigquery/commit/776ff1004592f62799ff0244a448d6911bcca5be)) * update dependency com.google.cloud:google-cloud-bigtable to v2.3.1 ([#1741](https://www.github.com/googleapis/java-bigquery/issues/1741)) ([2f31a0a](https://www.github.com/googleapis/java-bigquery/commit/2f31a0a4f491eca25cbd3992e48f94214bfd605b)) * update dependency com.google.cloud:google-cloud-bigtable to v2.4.0 ([#1746](https://www.github.com/googleapis/java-bigquery/issues/1746)) ([92e5d02](https://www.github.com/googleapis/java-bigquery/commit/92e5d02ff25511233b15f07844bb8b13de2dc72f)) * update dependency com.google.cloud:google-cloud-storage to v2.2.2 ([#1740](https://www.github.com/googleapis/java-bigquery/issues/1740)) ([2022301](https://www.github.com/googleapis/java-bigquery/commit/2022301b39390f20796b8c5b3d6ee0e82aa127aa)) * update jmh.version to v1.34 ([#1758](https://www.github.com/googleapis/java-bigquery/issues/1758)) ([5a2bcbc](https://www.github.com/googleapis/java-bigquery/commit/5a2bcbc7197fa75a464ed62d3e3df3bd43652b9d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…1879) #1744 introduced retrying 200 responses, but the regex that we are using is too broad: [".*exceed.*rate.limit."] and in some cases it may catch a valid response. The issue is that the current logic is scanning the whole response. In this PR the retry logic is modified to specifically check the error message from the response. For this purpose parsing logic was developed to extract error messages from responses. This logic is specifically designed for the [jobs.insert method response](https://cloud.google.com/bigquery/docs/reference/rest/v2/jobs/insert#response-body) (only case observed so far where a response with status code 200 might also return an error message).
fix(java): handle empty modules Fixes googleapis/synthtool#1743 Source-Link: googleapis/synthtool@482d649 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:bd5071596a47614d1fe15eb766c4255bae330f823b606e1196a3b0c8d2e96fd1
fix(java): handle empty modules Fixes googleapis/synthtool#1743 Source-Link: googleapis/synthtool@482d649 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:bd5071596a47614d1fe15eb766c4255bae330f823b606e1196a3b0c8d2e96fd1 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
With these changes Create Job will retry for rate limit exceeded even if the status code is 200