Add gRPC server and client samples with Spring Integration#377
Add gRPC server and client samples with Spring Integration#377cppwfs wants to merge 4 commits intospring-projects:mainfrom
Conversation
|
Looks promising. Thanks. |
artembilan
left a comment
There was a problem hiding this comment.
It also would be great to have tests for these new examples.
This way the samples project would serve a role of smoke tests for core project.
Thanks
gradle.properties
Outdated
| springBootVersion=4.1.0-SNAPSHOT | ||
| protobufVersion=4.29.4 | ||
| protobufPluginVersion=0.9.4 | ||
| grpcVersion=1.78.0 |
There was a problem hiding this comment.
That's not where we keep versions.
See ext block in the build.gradle.
And I guess we don't need protobufPluginVersion variable since it can simply go to the plugin definition itself.
build.gradle
Outdated
|
|
||
| dependencies { | ||
| implementation 'org.springframework.boot:spring-boot-starter-integration' | ||
| implementation "org.springframework.integration:spring-integration-grpc:7.1.0-SNAPSHOT" |
There was a problem hiding this comment.
Why do we need explicit SI version here?
More over there is a dedicated variable in the ext block.
build.gradle
Outdated
| //Test | ||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testImplementation 'org.springframework.integration:spring-integration-test' | ||
| testRuntimeOnly 'org.junit.platform:junit-platform-launcher' |
There was a problem hiding this comment.
If we need this, then it probably should go to the common deps section for all the projects.
build.gradle
Outdated
|
|
||
| //Test | ||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testImplementation 'org.springframework.integration:spring-integration-test' |
There was a problem hiding this comment.
I think this one has to be in the common deps section.
If that is not the case, we should consider to improve the project.
build.gradle
Outdated
|
|
||
| dependencies { | ||
| implementation 'org.springframework.boot:spring-boot-starter-integration' | ||
| implementation "org.springframework.integration:spring-integration-grpc:7.1.0-SNAPSHOT" |
There was a problem hiding this comment.
If you fix the server sample, I guess same has to be done for the client one, too.
| @@ -0,0 +1 @@ | |||
| spring.grpc.server.port=9090 No newline at end of file | |||
There was a problem hiding this comment.
New line in the end of each file.
I believe there supposed to be an option in IntelliJ IDEA.
| MessageChannel grpcInputChannelStreamResponse, | ||
| FluxMessageChannel grpcStreamOutputChannel) { | ||
| //TODO: Need advice, there has to be a better way than what I have below (or is this a bug in the | ||
| // GrpcOutboundGateway? |
There was a problem hiding this comment.
OK. Let's debug it together to see what is going on!
There was a problem hiding this comment.
Looks like we don't need this anymore.
Plus, the gateway could be created from the Grpc DSL factory.
| MessageChannel grpcInputChannelStreamResponse, | ||
| FluxMessageChannel grpcStreamOutputChannel) { | ||
| //TODO: Need advice, there has to be a better way than what I have below (or is this a bug in the | ||
| // GrpcOutboundGateway? |
There was a problem hiding this comment.
Looks like we don't need this anymore.
Plus, the gateway could be created from the Grpc DSL factory.
| * @author Glenn Renfro | ||
| */ | ||
| @SpringBootTest(properties = { | ||
| "spring.main.web-application-type=none", |
There was a problem hiding this comment.
If we don't use web, then this looks suspicious.
As well as the next line.
Why do we need to override some bean?
|
|
||
| @Bean | ||
| @Primary | ||
| public ApplicationRunner grpcClientStreamResponse() { |
There was a problem hiding this comment.
Are you sure we need to override these beans for test?
Feels like the sample is very complex, not something what users would expect coming here for beginner's knowledge
There was a problem hiding this comment.
Please, elaborate why these bean overrides were not removed.
| /** | ||
| * @author Glenn Renfro | ||
| */ | ||
| @SpringBootTest(webEnvironment = WebEnvironment.NONE) |
There was a problem hiding this comment.
I'm still not sure what web is doing in these samples...
gradle.properties
Outdated
| @@ -1,5 +1,6 @@ | |||
| version=7.1.0 | |||
| springBootVersion=4.1.0-SNAPSHOT | |||
| protobufVersion=4.29.4 | |||
There was a problem hiding this comment.
How that happened that grpcVersion has gone to ext, but not this?
| @BeforeEach | ||
| void setUp() { | ||
| Object server = ReflectionTestUtils.getField(this.grpcServerLifecycle, "server"); | ||
| this.grpcServerPort = ReflectionTestUtils.invokeMethod(server, "getPort"); |
There was a problem hiding this comment.
I think something more convenient has to be Spring gRPC, rather than such a hack with reflection.
| private static final Log LOG = LogFactory.getLog(ClientHelloWorldConfiguration.class); | ||
|
|
||
| /** | ||
| * Creates a managed gRPC channel for communication with the server. |
There was a problem hiding this comment.
Why not imperative?
This is a method and this is a sample.
Why give readers of these samples a wrong impression about Spring code style and consistency?
Everything what we do here is for community and Spring newcomers.
Would be great to teach them a right lesson and don't overcomplicate the sample code scary impression reasons.
So, see also about those blank lines in method Javadocs.
| */ | ||
| @Bean(destroyMethod = "shutdownNow") | ||
| ManagedChannel managedChannel(@Value("${grpc.server.host:localhost}") String host, | ||
| @Value("${grpc.server.port:9090}") int port) { |
There was a problem hiding this comment.
As we talked not one time: if method declaration is multi-line, a blank line before before method body.
I also wonder if there are some Spring gRPC auto-configuration for clients.
If there is really a reason to have our own ManagedChannel and those props with hard-coded defaults in the bean definition.
| * | ||
| * @param grpcInputChannelStreamResponse the message channel for streaming response requests | ||
| * @param grpcStreamOutputChannel channel that contains the responses | ||
| * @param replyTimeout the time in seconds to await for the response. Defaults to 1 second. |
There was a problem hiding this comment.
Why no consistency?
The previous example talks about milliseconds instead.
...rg/springframework/integration/samples/grpc/configuration/ClientHelloWorldConfiguration.java
Show resolved
Hide resolved
| .doOnSubscribe(subscription -> { | ||
| HelloRequest request = HelloRequest.newBuilder().setName("Jack").build(); | ||
| Message<?> requestMessage = MessageBuilder.withPayload(request).build(); | ||
| grpcInputChannelStreamResponse.send(requestMessage); |
There was a problem hiding this comment.
When we do setReplyChannel(replyChannel), this should go outside of the Flux handling.
We can subscribe() and then send: exactly the way how it is recommended by Project Reactor.
|
|
||
| package integration.grpc.hello; | ||
|
|
||
| option java_package = "org.springframework.integration.grpc.proto"; |
There was a problem hiding this comment.
We need to double check if these packages have a proper name.
Not sure if without a sample sub-package it is OK to deliver.
|
|
||
| @Bean | ||
| @Primary | ||
| public ApplicationRunner grpcClientStreamResponse() { |
There was a problem hiding this comment.
Please, elaborate why these bean overrides were not removed.
|
|
||
| @DynamicPropertySource | ||
| static void grpcServerProperties(DynamicPropertyRegistry registry) throws IOException { | ||
| mockGrpcServer = ServerBuilder.forPort(0) |
There was a problem hiding this comment.
That's not correct place to declare server instance.
Plus I wonder if it really should be a socket-based, but not InProcessServerBuilder, like we do in Spring Integration tests.
And another thought: I wonder if there is some Spring gRPC auto-configuration for testing to avoid a lot of boilerplate code like this to confuse end-users.
| @@ -0,0 +1 @@ | |||
| spring.grpc.server.port=9090 | |||
There was a problem hiding this comment.
Isn't that a default value?
Why do we need to declare this property here at all?
Probably mention Spring gRPC docs in the README to this sample should be enough.
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| this.grpcServerPort = this.grpcServerLifecycle.getPort(); |
There was a problem hiding this comment.
Doesn't look like we need the port as a property.
More over, I believe there should be some channel auto-configuration in Spring gRPC to avoid duplication.
Plus stubbing also can be done by Spring gRPC.
See their docs for more info.
Implement gRPC examples demonstrating Spring Integration's gRPC support for both server and client scenarios. Server implementation: - Add `GrpcInboundGateway` with service routing for multiple RPC patterns - Implement unary RPC and server streaming streaming methods - Configure HelloWorldService with message routing based on service method headers Client implementation: - Add `GrpcOutboundGateway` for unary and streaming response handling - Implement ApplicationRunner beans to demonstrate both communication patterns
- Removed POC experimental code
Replace manual channel/server setup in gRPC client and server samples with Spring gRPC's `GrpcChannelFactory` and in-process testing utilities to improve alignment with Spring conventions and reduce boilerplate. - Add `@EnableIntegration` to `ClientHelloWorldConfiguration` - Remove unused bidirectional and client-streaming RPC definitions from `hello.proto` in both client and server modules - Refactor `GrpcClientTests` to use an in-process gRPC server
8dbc2d6 to
25637a1
Compare
artembilan
left a comment
There was a problem hiding this comment.
Thanks for the update!
Very close, but still there are some questions.
| @SpringBootApplication | ||
| public final class Application { | ||
|
|
||
| private Application() { |
| */ | ||
| @Bean | ||
| ManagedChannel managedChannel(GrpcChannelFactory factory) { | ||
| return factory.createChannel("local"); |
There was a problem hiding this comment.
Isn't there an auto-configuration in Spring gRPC on the matter?
| syntax = "proto3"; | ||
| import "types.proto"; | ||
|
|
||
| package integration.grpc.hello; |
There was a problem hiding this comment.
Do we need to revise this package as well?
| /** | ||
| * @author Glenn Renfro | ||
| */ | ||
| @SpringBootTest(properties = {"spring.grpc.server.enabled=false"}) |
There was a problem hiding this comment.
Why do we do this instead of auto-configuration suggested by Spring gRPC?
|
|
||
| dependencies { | ||
| implementation 'org.springframework.boot:spring-boot-starter-integration' | ||
| implementation "org.springframework.integration:spring-integration-grpc:$springIntegrationVersion" |
There was a problem hiding this comment.
I don't think we need an explicit version for Spring Integration dependencies since we rely on Spring Boot management.
| @SpringBootApplication | ||
| public final class Application { | ||
|
|
||
| private Application() { |
There was a problem hiding this comment.
Also suspicious ctor.
How it happened here?
I never saw start.spring.io does that for us.
|
|
||
| @Bean | ||
| GrpcInboundGateway helloWorldService() { | ||
| GrpcInboundGateway gateway = new GrpcInboundGateway(HelloWorldServiceGrpc.HelloWorldServiceImplBase.class); |
There was a problem hiding this comment.
I don't think that is what we want here in the test.
I believe just regular gRPC HelloWorldServiceImplBase implementation as a separate class in this test scope would be enough.
I mean that is fine that we verify Spring Integration server side, but that should not be a goal of the test in the sample for client side.
| return flow -> flow | ||
| .handle(Grpc.outboundGateway(managedChannel, HelloWorldServiceGrpc.class) | ||
| .methodName("StreamSayHello")) | ||
| .channel(grpcStreamOutputChannel); |
There was a problem hiding this comment.
I see you do .setReplyChannel(grpcStreamOutputChannel) in the respective ApplicationRunner impl.
So, this one must not be here, but just fully similar to the grpcOutboundFlowSingleResponse configuration.
|
|
||
| ```properties | ||
| grpc.server.host=localhost | ||
| grpc.server.port=9090 |
There was a problem hiding this comment.
May it worth to have such an application.properties file in our sample?
|
|
||
| $ gradlew :grpc-server:bootRun | ||
|
|
||
| The server will start on port 9090 by default (configured in `application.properties`). |
There was a problem hiding this comment.
I see we do have such a file in the sample, but it is empty.
Should we add a respective property to let end-user to override it to something they want to play with?
|
I'm going to close this PR. I want to tackle it from a different angle from the current solution. |
Implement gRPC examples demonstrating Spring Integration's gRPC support for both server and client scenarios.
Server implementation:
GrpcInboundGatewaywith service routing for multiple RPC patternsClient implementation:
GrpcOutboundGatewayfor unary and streaming response handling