-
Notifications
You must be signed in to change notification settings - Fork 127
feat(bigquery): Add custom ExceptionHandler to BigQueryOptions #3937
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
base: main
Are you sure you want to change the base?
Conversation
@@ -118,6 +124,24 @@ public Builder setOpenTelemetryTracer(Tracer tracer) { | |||
return this; | |||
} | |||
|
|||
public Builder abortOn(Class<? extends Exception> exception) { |
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.
Providing the helper methods abortOn
and retryOn
in BigQueryOptions is probably easier to use for the user at the cost of flexibility. The sample code to use current feature would look something like:
BigQuery bigQuery =
BigQueryOptions.newBuilder()
.abortOn(RuntimeException.class)
.retryOn(java.net.UnknownHostException.class)
...
.build()
.getService();
Consider allowing users to directly set the ResultRetryAlgorithm in BigQueryOption so it might look like this:
BigQuery bigQuery =
BigQueryOptions.newBuilder()
.setResultRetryAlgorithm(
ExceptionHandler.newBuilder().
.abortOn(RuntimeException.class)
.retryOn(java.net.UnknownHostException.class)
.addInterceptors(EXCEPTION_HANDLER_INTERCEPTOR))
...
.build()
.getService();
We can also consider adding a BigQueryExceptionHandler class to help the user not have to set addInterceptors()
by providing a BigQueryExceptionHandler so the final code might look like this:
BigQuery bigQuery =
BigQueryOptions.newBuilder()
.setResultRetryAlgorithm(
BigQueryExceptionHandler.newBuilder().
.abortOn(RuntimeException.class)
.retryOn(java.net.UnknownHostException.class)
.build())
...
.build()
.getService();
Let me know what you think,
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 think I still prefer to just set the retry/abort exceptions directly within the BigQueryOptions
builder. Unless the user defines their own ResultRetryAlgorithm
object, they are gaining no new flexibility or customization compared to setting the abort/retry settings directly in the options as there are no other builder options to set in ExceptionHandler
. Having them create an ExceptionHandler
object in the builder accomplishes the same thing but with more verbosity (though you could argue this makes the code more readable).
If we do decide to make them specify the algorithm instead of just the .abortOn()/.retryOn()
helpers, I think the BigQueryExceptionHandler
is the better option since the Interceptor
interface is not very clear. This means writing and managing a new class that has essentially the same functionality as ExceptionHandler
without providing any extra functionality or ease of use compared to the .abortOn()/.retryOn()
helpers in BigQueryOptions
.
This change already enables more flexibility to the user than they had by choosing which exceptions to abort and retry on. I suppose I don't expect users to want (or need) to define their own ResultRetryAlgorithm
objects since it comes from gax and feels like an internal implementation detail to me. But I could be very wrong in this regard, do you foresee users wanting to specify their own ResultRetryAlgorithm
objects?
This PR adds in methods to the BigQueryOptions builder to allow users to decide which types of exception to abort and retry on. If no exceptions are supplied, the retry algorithm defaults to the current hardcoded retry algorithm.
Fixes b/429414790 (internal)