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: optimize unary callables to not wait for trailers #1356

Merged
merged 13 commits into from Jul 19, 2021

Conversation

igorbernstein2
Copy link
Collaborator

@igorbernstein2 igorbernstein2 commented Apr 29, 2021

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

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
@igorbernstein2 igorbernstein2 requested a review from vam-google Apr 29, 2021
@google-cla google-cla bot added the cla: yes label Apr 29, 2021
@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented May 5, 2021

@vam-google when you have a moment, can you comment on the approach? if it looks good, I'll update tests & docs

try {
clientCall.sendMessage(request);
clientCall.halfClose();
// Request an extra message to detect misconfigured servers
Copy link
Contributor

@vam-google vam-google May 18, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 Jun 16, 2021

Choose a reason for hiding this comment

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

I wanted to minimize the behavioral differences and its fairly cheap. If you feel strongly, I can remove it. let me know

Copy link
Contributor

@vam-google vam-google Jul 19, 2021

Choose a reason for hiding this comment

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

Behavioral differences between what and what? I would still recommend removing it though.

Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 Jul 19, 2021

Choose a reason for hiding this comment

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

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

@igorbernstein2 igorbernstein2 changed the title feat: optimize unary callables to not wait for trailers [draft] feat: optimize unary callables to not wait for trailers Jun 16, 2021
@igorbernstein2 igorbernstein2 marked this pull request as ready for review Jun 16, 2021
@igorbernstein2 igorbernstein2 requested review from as code owners Jun 16, 2021
@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Jun 16, 2021

@vam-google I think addressed all comments, PTAL

@elharo
Copy link
Contributor

@elharo elharo commented Jun 16, 2021

You need to run the formatter:

FAILURE: Build failed with an exception.

Task :verifyGoogleJavaFormat FAILED

  • What went wrong:
    Execution failed for task ':verifyGoogleJavaFormat'.

Problems: formatting style violations

Copy link
Contributor

@vam-google vam-google left a comment

LGTM with few minor comments and under assumption that by default it is disabled (alwaysAwaitTrailers as a settings property is false by default, is it correct?)

try {
clientCall.sendMessage(request);
clientCall.halfClose();
// Request an extra message to detect misconfigured servers
Copy link
Contributor

@vam-google vam-google Jul 19, 2021

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

@vam-google vam-google Jul 19, 2021

Choose a reason for hiding this comment

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

So this is is not wrapped with ListeneableFutureToApiFuture why?

Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 Jul 19, 2021

Choose a reason for hiding this comment

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

GrpcClientCalls is owned by gax, so it already returns an ApiFuture, whereas ClientCalls is owned by grpc returns a ListenableFuture

@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Jul 19, 2021

LGTM with few minor comments and under assumption that by default it is disabled (alwaysAwaitTrailers as a settings property is false by default, is it correct?)

Yep, its off by default

@igorbernstein2 igorbernstein2 merged commit dd5f955 into googleapis:master Jul 19, 2021
7 checks passed
@igorbernstein2 igorbernstein2 deleted the optimize-trailers branch Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants