Add client and server support for gRPC deadlines #1462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bondolo
force-pushed
the
deadline-grpc
branch
2 times, most recently
from
549a5a5 to
520b63f
Compare
idelpivnitskiy
requested changes
Apr 3, 2021
servicetalk-examples/docs/modules/ROOT/pages/_partials/nav-versioned.adoc
Outdated
Show resolved
Hide resolved
...amples/grpc/deadline/src/main/java/io/servicetalk/examples/grpc/deadline/DeadlineServer.java
Outdated
Show resolved
Hide resolved
...amples/grpc/deadline/src/main/java/io/servicetalk/examples/grpc/deadline/DeadlineClient.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java
Show resolved
Hide resolved
|
The overall approach looks great! My comments mostly about details |
bondolo
commented
Apr 3, 2021
bondolo
force-pushed
the
deadline-grpc
branch
2 times, most recently
from
3e16055 to
34b4cb2
Compare
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcMetadata.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcClientCallFactory.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcUtils.java
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcUtils.java
Outdated
Show resolved
Hide resolved
Scottmitch
reviewed
Apr 14, 2021
| // demonstration purposes. | ||
| CountDownLatch responseProcessedLatch = new CountDownLatch(2); | ||
|
|
||
| // Make a request using default timeout (this will succeed) |
There was a problem hiding this comment.
nit: consider simplifying example and remove this case which doesn't specify the timeout. setting the default timeout on the builder + comments should be sufficient.
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcClientCallFactory.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcClientCallFactory.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcClientCallFactory.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/DefaultGrpcClientMetadata.java
Outdated
Show resolved
Hide resolved
| * gRPC timeout is stored in context as a deadline so that when propagated to a new request the remaining time to be | ||
| * included in the request can be calculated. | ||
| */ | ||
| protected static final AsyncContextMap.Key<Instant> GRPC_DEADLINE_KEY = PKG_GRPC_DEADLINE_KEY; |
There was a problem hiding this comment.
can we keep AsyncContext usage internal? ideally this doesn't have to be exposed in the public API as we may switch to using RequestContext in the future.
There was a problem hiding this comment.
Doing so would require adding another module "servicetalk-grpc-internal' and a class to share the "secret" async context key between modules. I avoided that for now on the expectation that there aren't going to be user implementations of the GrpcServerBuilder and GrcpcClientBuilder
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcServerBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-api/src/main/java/io/servicetalk/grpc/api/GrpcUtils.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcClientBuilder.java
Outdated
Show resolved
Hide resolved
servicetalk-grpc-netty/src/main/java/io/servicetalk/grpc/netty/DefaultGrpcServerBuilder.java
Outdated
Show resolved
Hide resolved
Motivation: Some gRPC usages benefit from being able to limit the amount of time a gRPC call is allowed to execute. Modifications: Features are added for time limiting gRPC calls. Result: Execution time for gRPC can be more easily managed.
The gRPC deadline async context key is now shared among all uses Cleanup some of the error generation/translation code for cleaner sharing
|
LGTM, thanks for the great work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Some gRPC usages require completion of calls within specified time limits. The gRPC protocol provides capability for specifying timeout (aka deadline) for completion of calls.
Modifications:
Features are added to support specification of call timeouts.
Result:
ServiceTalk gRPC API now supports gRPC timeout functionality.