Skip to content
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: support setting timeout per RPC #379

Merged
merged 4 commits into from Aug 25, 2020
Merged

feat: support setting timeout per RPC #379

merged 4 commits into from Aug 25, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Aug 10, 2020

The Spanner client allows a user to set custom timeouts while creating a SpannerOptions instance, but these timeouts are static and are applied to all invocations of the RPCs. This change introduces the possibility to set custom timeouts and other call options on a per-RPC basis.

This feature will also be very useful for the Async Connection API, as it will allow applying the statement timeouts for async calls on a per RPC-invocation basis.

Fixes #378

The Spanner client allows a user to set custom timeouts while creating a
SpannerOptions instance, but these timeouts are static and are applied to
all invocations of the RPCs. This change introduces the possibility to set
custom timeouts and other call options on a per-RPC basis.

Fixes #378
@google-cla google-cla bot added the cla: yes label Aug 10, 2020
@hengfengli
Copy link
Contributor

@hengfengli hengfengli commented Aug 17, 2020

@olavloite Do you have any idea why ci / clirr fails?

Copy link
Contributor

@hengfengli hengfengli left a comment

Generally, using Context looks okay to me. My concerns are:

  1. Handwritten code need to be maintained. E.g., we need to call different timeout setting methods to different RPCs. If we need to add a new RPC, we need to add the timeout and its related methods here.
  2. I wonder if there is a way to rely on the underlying gax-java and gapic generated code to implement this feature instead of using Context? E.g.,
client
    .singleUse()
    .executeQuery(
        Statement.of(
            "SELECT SingerId, FirstName, LastName FROM Singers ORDER BY LastName")
        ),
        callOptions,      // This includes timeout and retry policy? 
)

The above code is my imagination according to the Ruby version: https://github.com/googleapis/google-cloud-ruby/blob/4dcf29950885d8ac3dec41d219f06b843c638a8e/google-cloud-spanner/lib/google/cloud/spanner/client.rb#L351-L372

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Aug 17, 2020

Generally, using Context looks okay to me. My concerns are:

  1. Handwritten code need to be maintained. E.g., we need to call different timeout setting methods to different RPCs. If we need to add a new RPC, we need to add the timeout and its related methods here.

Yes, that is correct, but I'm afraid that there is no real way of circumventing that in the Java client. The Java client really hides away most gRPC details, especially for the data client, so if we want to support RPC specific timeouts, it will always include some handwritten code. Note that the SpannerCallContextTimeoutConfigurator class is only a convenience class to support users when setting a timeout for the data client. It is not strictly necessary, and I deliberately skipped the same convenience class for the admin clients to reduce the amount of handwritten code.

  1. I wonder if there is a way to rely on the underlying gax-java and gapic generated code to implement this feature instead of using Context? E.g.,
client
    .singleUse()
    .executeQuery(
        Statement.of(
            "SELECT SingerId, FirstName, LastName FROM Singers ORDER BY LastName")
        ),
        callOptions,      // This includes timeout and retry policy? 
)

The above code is my imagination according to the Ruby version: https://github.com/googleapis/google-cloud-ruby/blob/4dcf29950885d8ac3dec41d219f06b843c638a8e/google-cloud-spanner/lib/google/cloud/spanner/client.rb#L351-L372

This solution does use CallOptions and gax-java for configuring the timeout, but the issue here is that the java client does not expose any of that in its public methods, which is why I chose to let the user specify it as a context value. It would also be possible to do it in a similar way as the Ruby client by allowing the user to specify CallOptions for each method, but that would mean changing the public interface of virtually every method to allow the user to supply additional CallOptions. That feels like a very large change for what is in the end a feature aimed at advanced users.

Copy link
Contributor

@hengfengli hengfengli left a comment

LGTM. Nits: there is an unresolved comment.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Aug 17, 2020

LGTM. Nits: there is an unresolved comment.

Sorry, missed that one, but fixed now.

@product-auto-label product-auto-label bot added the api: spanner label Aug 21, 2020
if (spannerMethod == null) {
return null;
}
switch (SpannerMethod.valueOf(request, method)) {
Copy link
Contributor

@thiagotnunes thiagotnunes Aug 24, 2020

Choose a reason for hiding this comment

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

nit (don't have to change this, just a suggestion): it might be easier to test this / encapsulate the logic if we use a visitor pattern.

Copy link
Contributor Author

@olavloite olavloite Aug 25, 2020

Choose a reason for hiding this comment

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

That's probably a good idea. We would still need some kind of lookup table for the timeout / method combination (i.e. keeping them in a hash map or something), but it might make it somewhat simpler. I'll do that in a separate PR, as it won't change the public interface of it.

@olavloite olavloite merged commit 5d115d4 into master Aug 25, 2020
15 checks passed
@olavloite olavloite deleted the timeout-per-rpc branch Aug 25, 2020
thiagotnunes pushed a commit to thiagotnunes/java-spanner that referenced this issue Jun 5, 2021
* feat: support setting timeout per RPC

The Spanner client allows a user to set custom timeouts while creating a
SpannerOptions instance, but these timeouts are static and are applied to
all invocations of the RPCs. This change introduces the possibility to set
custom timeouts and other call options on a per-RPC basis.

Fixes googleapis#378

* fix: change grpc deps from test to compile scope

* fix: address review comments

* fix: resolve review comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants