add grpc client autowired #1309
Conversation
Signed-off-by: seal90 <[email protected]>
| * Handlers Dapr gRPC client in spring boot. | ||
| * Searches for fields and methods in beans that are annotated with {@link DaprGrpcClient} and sets them. | ||
| * | ||
| * @author seal90 ([email protected]) |
There was a problem hiding this comment.
@seal90 we are not using author tags here.
| @@ -21,6 +21,12 @@ | |||
| <version>${project.parent.version}</version> | |||
| <optional>true</optional> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
@seal90 why do we need a new module?
There was a problem hiding this comment.
In my head a new module makes sense if we want to isolate the gRPC dependencies because we think they might clash with other dependencies on the client application.
There was a problem hiding this comment.
The abilities possessed by DAPR：service invocation, publish/subscribe, workflow, statemanagement, bindings...
dapr-spring model: data(state), messageing, workflows
example page: https://github.com/dapr/java-sdk/tree/master/examples/src/main/java/io/dapr/examples
I think we split the module by abilitie, so i create a new module.
| public class DaprInvokeAutoConfiguration { | ||
|
|
||
| @Bean | ||
| @ConditionalOnProperty(name = "dapr.invoke.grpc.client.daprGrpcClient.enabled", havingValue = "true", |
There was a problem hiding this comment.
@seal90 what's the use case for this? When will people enable this property?
There was a problem hiding this comment.
I wonder if instead of having this, we have something like @EnableDaprWorkflows, but looking at this example: https://www.baeldung.com/spring-boot-grpc, there is no need to disable this.
There was a problem hiding this comment.
This feature is enabled by default. For users who only use the HTTP protocol, there will be no impact as they do not use @ DaprGrpcClient.
Due to the possibility that the matching logic initialized below may not be able to handle new API issues, such ashttps://github.com/grpc-ecosystem/grpc-spring/issues/1177. If the user is in a hurry to use the new API, they can close this area and implement this logic again on their own.
private <T extends AbstractStub<T>> T createStub(final Class<T> stubClass, final Channel channel) {
try {
// Search for public static *Grpc#new*Stub(Channel)
final Class<?> declaringClass = stubClass.getDeclaringClass();
if (declaringClass != null) {
for (final Method method : declaringClass.getMethods()) {
final String name = method.getName();
final int modifiers = method.getModifiers();
final Parameter[] parameters = method.getParameters();
if (name.startsWith("new") && name.endsWith("Stub")
&& Modifier.isStatic(modifiers) && Modifier.isPublic(modifiers)
&& method.getReturnType().isAssignableFrom(stubClass)
&& parameters.length == 1
&& Channel.class.equals(parameters[0].getType())) {
return stubClass.cast(method.invoke(null, channel));
}
}
}
// Search for a public constructor *Stub(Channel)
final Constructor<T> constructor = stubClass.getConstructor(Channel.class);
return constructor.newInstance(channel);
} catch (final Exception e) {
throw new BeanInstantiationException(stubClass, "Failed to create gRPC client", e);
}
}
| @RestController | ||
| public class HelloWorldController { | ||
|
|
||
| @DaprGrpcClient("producer-app") |
There was a problem hiding this comment.
@seal90 ok.. I think I understand what is going on here..
Looking at this example: https://www.baeldung.com/spring-boot-grpc having it named @DaprGrpcClient makes sense.
There was a problem hiding this comment.
The idea from https://github.com/grpc-ecosystem/grpc-spring.
At first, I wanted to expand its capabilities, but I found that it hasn't changed in a year, but this usage method is really useful.
|
@seal90 can you please check the CI pipelines? we need to make sure that they don't fail with the new changes. |
Okay, I'm working on it. |
Signed-off-by: seal90 <[email protected]>
…to grpc_announce
Signed-off-by: seal90 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1309 +/- ##
============================================
- Coverage 76.91% 75.30% -1.62%
- Complexity 1592 1620 +28
============================================
Files 145 196 +51
Lines 4843 5114 +271
Branches 562 555 -7
============================================
+ Hits 3725 3851 +126
- Misses 821 947 +126
- Partials 297 316 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| @Override | ||
| public Object postProcessBeforeInitialization(final Object bean, final String beanName) throws BeansException { | ||
| processDaprGprcClients(bean); |
There was a problem hiding this comment.
Is the checkstyle working?
|
@seal90 to be honest I am not sure why we need this complexity. A GRPC stub is domain and application specific. If a user wants to create Spring bean for a GRPC stub, this is totally doable although with a little bit of boilerplate. And to echo @salaboy's concerns we don't need yet another Maven module for GRPC invocation functionality, the infrastructure we already have is enough for that. |
|
@artur-ciocanu i thought the same at the beginning but here , the grpc client is using the dapr channel to talk to the grpc stub.. at least that was my understanding. I might need to review this again. If this is actually doing that, having a new module makes sense to isolate the grpc dependencies.. but let's double check before suggesting more changes in this PR |
Description
add grpc client autowired @DaprGrpcClient
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1308
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: