feat: optimize unary callables to not wait for trailers #1356
Conversation
gRPC ClientCalls and thus gax currently wait for trailers to resolve unary call futures. I believe the original reason for this was to mitigate misconfigured servers where a server endpoint was changed to be server streaming, but the client still expects a unary method. We measured the cost of this safety net to be O(hundreds of millis). For low latency services like Bigtable, this is very high. This PR is incomplete, but is meant to be a conversation starter. I would like to get gax's opinion on this and guidance how to proceed. Some initial proposals: 1. productionize this PR and roll it out 2. gate this behavior using a flag in UnaryCallSettings 3. expose a bit more surface in gax to allow cloud bigtable to build our callable chains (the current blocker is that GrpcUnaryRequestParamCallable & GrpcExceptionCallable are package private
google-cla
bot
added
the
cla: yes
|
@vam-google when you have a moment, can you comment on the approach? if it looks good, I'll update tests & docs |
gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallSettings.java
Outdated
Show resolved
Hide resolved
| try { | ||
| clientCall.sendMessage(request); | ||
| clientCall.halfClose(); | ||
| // Request an extra message to detect misconfigured servers |
There was a problem hiding this comment.
Should client libraries really consider the cases of the server side being misconfigured? Keeping server-side correct should be the responsibility of the server-side implementation and should not propagate to client side.
There was a problem hiding this comment.
I wanted to minimize the behavioral differences and its fairly cheap. If you feel strongly, I can remove it. let me know
There was a problem hiding this comment.
Behavioral differences between what and what? I would still recommend removing it though.
There was a problem hiding this comment.
Difference between grpc behavior and this. It will also ensure that the call is properly closed as opposed to hanging indefinitely if a second message is sent. Overall I think it makes debugging easier
gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcClientCalls.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcClientCalls.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcEagerDirectCallable.java
Outdated
Show resolved
Hide resolved
igorbernstein2
changed the title
|
@vam-google I think addressed all comments, PTAL |
|
You need to run the formatter: FAILURE: Build failed with an exception.
|
| try { | ||
| clientCall.sendMessage(request); | ||
| clientCall.halfClose(); | ||
| // Request an extra message to detect misconfigured servers |
There was a problem hiding this comment.
Behavioral differences between what and what? I would still recommend removing it though.
| if (awaitTrailers) { | ||
| return new ListenableFutureToApiFuture<>(ClientCalls.futureUnaryCall(clientCall, request)); | ||
| } else { | ||
| return GrpcClientCalls.eagerFutureUnaryCall(clientCall, request); |
There was a problem hiding this comment.
So this is is not wrapped with ListeneableFutureToApiFuture why?
There was a problem hiding this comment.
GrpcClientCalls is owned by gax, so it already returns an ApiFuture, whereas ClientCalls is owned by grpc returns a ListenableFuture
Yep, its off by default |
🤖 I have created a release \*beep\* \*boop\* --- ## [1.67.0](https://www.github.com/googleapis/gax-java/compare/v1.66.0...v1.67.0) (2021-07-19) ### Features * introduce closeAsync to Batcher ([#1423](https://www.github.com/googleapis/gax-java/issues/1423)) ([aab5288](https://www.github.com/googleapis/gax-java/commit/aab528803405c2b5f9fc89641f47abff948a876d)) * optimize unary callables to not wait for trailers ([#1356](https://www.github.com/googleapis/gax-java/issues/1356)) ([dd5f955](https://www.github.com/googleapis/gax-java/commit/dd5f955a3ab740c677fbc6f1247094798eb814a3)) * update DirectPath environment variables ([#1412](https://www.github.com/googleapis/gax-java/issues/1412)) ([4f63b61](https://www.github.com/googleapis/gax-java/commit/4f63b61f1259936aa4a1eaf9162218c787b92f2a)) ### Bug Fixes * remove `extends ApiMessage` from `HttpJsonStubCallableFactory` definition ([#1426](https://www.github.com/googleapis/gax-java/issues/1426)) ([87636a5](https://www.github.com/googleapis/gax-java/commit/87636a5812874a77e9004aab07607121efa43736)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
gRPC ClientCalls and thus gax currently wait for trailers to resolve unary call futures. I believe the original reason for this was to mitigate misconfigured servers where a server endpoint was changed to be server streaming, but the client still expects a unary method. We measured the cost of this safety net to be O(hundreds of millis). For low latency services like Bigtable, this is very high.
This PR is incomplete, but is meant to be a conversation starter. I would like to get gax's opinion on this and guidance how to proceed. Some initial proposals: