-
Notifications
You must be signed in to change notification settings - Fork 40
Improve GrpcChannelFactory API #78
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
Conversation
spring-grpc-core/src/main/java/org/springframework/grpc/client/GrpcChannelBuilder.java
Outdated
Show resolved
Hide resolved
spring-grpc-core/src/main/java/org/springframework/grpc/client/GrpcChannelFactory.java
Show resolved
Hide resolved
spring-grpc-core/src/main/java/org/springframework/grpc/client/DefaultGrpcChannelFactory.java
Show resolved
Hide resolved
spring-grpc-core/src/main/java/org/springframework/grpc/client/DefaultGrpcChannelFactory.java
Show resolved
Hide resolved
spring-grpc-core/src/main/java/org/springframework/grpc/client/DefaultGrpcChannelFactory.java
Show resolved
Hide resolved
spring-grpc-core/src/main/java/org/springframework/grpc/client/DefaultGrpcChannelFactory.java
Show resolved
Hide resolved
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.
@dsyer I believe this extended builder concept solves several of our current pain points.
Note
I bring the following point up now because it could impact the new extended channel builder design in this code proposal.
I noticed that the NettyChannelBuilder
static methods handle the unix
scheme, for example:
if (DOMAIN_SOCKET_ADDRESS_SCHEME.equals(address.getScheme())) {
final String path = GrpcUtils.extractDomainSocketAddressPath(address.toString());
return NettyChannelBuilder.forAddress(new DomainSocketAddress(path))
.channelType(EpollDomainSocketChannel.class)
.eventLoopGroup(new EpollEventLoopGroup());
} else {
return NettyChannelBuilder.forTarget(address.toString())
.defaultLoadBalancingPolicy(properties.getDefaultLoadBalancingPolicy());
}
- Does the
Grpc.newChannelBuilder()
we are currently leveraging supportunix
scheme for Netty? - Refresh my memory why we did not have the
createChannel
return the specificManagedChannelBuilder
(e.g.NettyChannelBuilder
)? Was it for simplicity?
At the time I wrote it I believed not (copied code from ecosystem), but looking at the
I didn't want the user to have to know what kind of channel it is (same for servers). Mainly because I anticipated substituting the in-process version in tests. Maybe that was a bad decision - we'd have to support the in-process transport in a different way if we change the contract. |
473a742
to
cc7c249
Compare
spring-grpc-core/src/main/java/org/springframework/grpc/client/GrpcChannelFactory.java
Show resolved
Hide resolved
spring-grpc-core/src/main/java/org/springframework/grpc/client/ChannelBuilderOptions.java
Show resolved
Hide resolved
This commit improves the GrpcChannelFactory createChannel API by introducing `ChannelBuilderOptions` that can be specified during channel creation. Additionally, concrete Netty channel factory implementations have been added as well as adding type to the GrpcChannelBuilderCustomizer which helps match customizers to channel factories.
946c013
to
0771303
Compare
LGTM. Maybe we should use the overloaded default factory method in the tests where possible though (no need for the second argument if it’s just the default)? |
ChannelBuilderOptions
that holds customizer, shutdownGracePeriod, and interceptors.