-
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
Changes from all commits
16c606c
f4d9cb6
af20e7e
124116b
76cd092
84ae60f
2482f78
514b57a
b30f7c2
ca2931c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,7 +72,8 @@ public boolean shouldRetry( | |
// the exception messages | ||
boolean shouldRetry = | ||
(shouldRetryBasedOnResult(context, previousThrowable, previousResponse) | ||
|| shouldRetryBasedOnBigQueryRetryConfig(previousThrowable, bigQueryRetryConfig)) | ||
|| shouldRetryBasedOnBigQueryRetryConfig( | ||
previousThrowable, bigQueryRetryConfig, previousResponse)) | ||
&& shouldRetryBasedOnTiming(context, nextAttemptSettings); | ||
|
||
if (LOG.isLoggable(Level.FINEST)) { | ||
|
@@ -92,13 +93,26 @@ public boolean shouldRetry( | |
} | ||
|
||
private boolean shouldRetryBasedOnBigQueryRetryConfig( | ||
Throwable previousThrowable, BigQueryRetryConfig bigQueryRetryConfig) { | ||
Throwable previousThrowable, | ||
BigQueryRetryConfig bigQueryRetryConfig, | ||
ResponseT previousResponse) { | ||
/* | ||
We are deciding if a given error should be retried on the basis of error message. | ||
Cannot rely on Error/Status code as for example error code 400 (which is not retriable) could be thrown due to rateLimitExceed, which is retriable | ||
*/ | ||
String errorDesc; | ||
if (previousThrowable != null && (errorDesc = previousThrowable.getMessage()) != null) { | ||
String errorDesc = null; | ||
if (previousThrowable != null) { | ||
errorDesc = previousThrowable.getMessage(); | ||
} else if (previousResponse != null) { | ||
/* | ||
In some cases error messages may come without an exception | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Please add a mock test in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added mock test to |
||
} | ||
|
||
if (errorDesc != null) { | ||
errorDesc = errorDesc.toLowerCase(); // for case insensitive comparison | ||
for (Iterator<String> retriableMessages = | ||
bigQueryRetryConfig.getRetriableErrorMessages().iterator(); | ||
|
@@ -161,7 +175,8 @@ public TimedAttemptSettings createNextAttempt( | |
if (!((shouldRetryBasedOnResult(context, previousThrowable, previousResponse) | ||
|| shouldRetryBasedOnBigQueryRetryConfig( | ||
previousThrowable, | ||
bigQueryRetryConfig)))) { // Calling shouldRetryBasedOnBigQueryRetryConfig to check if | ||
bigQueryRetryConfig, | ||
previousResponse)))) { // Calling shouldRetryBasedOnBigQueryRetryConfig to check if | ||
// the error message could be retried | ||
return null; | ||
} | ||
|
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.