grpc-ecosystem / grpc-spring

Spring Boot starter module for gRPC framework.
https://grpc-ecosystem.github.io/grpc-spring/
Apache License 2.0
3.54k stars 830 forks source link

Support *not* autoconfiguring client and server when address starts with "none" #419

Open asarkar opened 4 years ago

asarkar commented 4 years ago

The problem

Currently, annotations @GrpcService and @GrpcClient autoconfigure the server and the client respectively. For integration testing, we can use in process server and client as explained here. In my case, I've a client-A that calls service-A that calls client-B that calls external-service-B. Depending on the test scenario, I substitute in process versions for service-A and external-service-B to test the flow, or use the real ones.

What I'd like to do now is use the same test for calling service-A deployed in my integration environment. That means, I only need in process client-A, and service-A and client-B beans shouldn't be created.

The solution

Just like in process is given special treatment now, it'd be nice to have the library support this use case by not configuring client/server that begins with none in their address. I'd point client-A to the remote service-A deployed in my integration environment and run the same test I run when I use local service-A.

Alternatives considered

One way is to use a Spring profile with @GrpcService in a @Configuration class, such the the beans are created only if Spring profile integration isn't active. That's fine, but it makes the Production code coupled with test code.

Another option is to simply let service-A and client-B beans be created, but not use them. This is very confusing and could lead to bugs since we have no way to tell whether client-A is talking to local service-A, or remote one.

ST-DDT commented 4 years ago

Let's check whether I got you right.

As for the client, why do you wish it not to be created instead of pointing to a arbitary unavailable address? As long as it is not used it is quite cheap AFAICT. And IMO dealing with NPEs is far more troublesome than an UNAVAILABLE error. Or do you wish to have a NameResolver that resolves the none: scheme? As for the server, I'm not sure what you wish to achieve with that. What would be the added benefit over disabling the related autoconfig altogether.

asarkar commented 4 years ago

why do you wish it not to be created instead of pointing to a arbitrary unavailable address?

I don't want to point to an arbitrary address, not sure why you think that, null is fine, which is same behavior you get if you use @Autowired(required = false). It's not about the stub being expensive or cheap, it's about avoiding errors; a NPE is clear indication that the reference is not supposed to be used.

As for the server, even more reason to not have a gRPC server running with services that I don't need. It's the same idea as testing a "slice" of the application using Spring Boot.

Spring Boot’s auto-configuration system works well for applications but can sometimes be a little too much for tests. It often helps to load only the parts of the configuration that are required to test a “slice” of your application.

https://docs.spring.io/spring-boot/docs/2.1.6.RELEASE/reference/html/boot-features-testing.html#boot-features-testing-spring-boot-applications-testing-autoconfigured-tests

ST-DDT commented 4 years ago

why do you wish it not to be created instead of pointing to a arbitrary unavailable address?

I don't want to point to an arbitrary address, not sure why you think that

You probably got me wrong here, but nevermind. The explanation still told me what I needed to know and I accept it as a feature request.

As for the server, I'm not sure if I prefer the address none or the autoconfigure exclude variant.

asarkar commented 4 years ago

I'm not sure if I prefer the address none or the autoconfigure exclude variant

Note that disabling the autoconfig is a global switch, whereas address none allows for more fine-grained control, where user may want to turn off one but keep another.

ST-DDT commented 4 years ago

I'm not sure if I prefer the address none or the autoconfigure exclude variant

Note that disabling the autoconfig is a global switch, whereas address none allows for more fine-grained control, where user may want to turn off one but keep another.

It is possible to exclude a single autoconfig:

spring.autoconfigure.exclude=net.devh.boot.grpc.server.autoconfigure.GrpcServerFactoryAutoConfiguration

Also it might already be possible by using:

grpc.server.port=-1
# and not configuring
#grpc.server.in-process-name=
asarkar commented 4 years ago

spring.autoconfigure.exclude

This I knew; what I meant is it's not possible this way to disable one service, but keep another. This turns off all gRPC service autoconfig.

grpc.server.port=-1

This, I think, is what I'm looking for; however, there's no support for this on the client side, and so it won't be consistent if we use different ways for server and client. Also, setting the port as -1 to disable service is not exactly intuitive.

ST-DDT commented 4 years ago

spring.autoconfigure.exclude

This I knew; what I meant is it's not possible this way to disable one service, but keep another. This turns off all gRPC service autoconfig.

GrpcServerFactoryAutoConfiguration is only for the actual server factories + their lifecycle. it does not disable the other beans such as the global interceptor discovery. As a result both properties do the exact same thing.

grpc.server.port=-1

This, I think, is what I'm looking for; however, there's no support for this on the client side, and so it won't be consistent if we use different ways for server and client. Also, setting the port as -1 to disable service is not exactly intuitive.

Well it's the first thing that came to my mind when I implemented it. So for you its important that both the server and the client side support none value for the address property? Then I should probably deprecate the port=-1 feature. Any suggestion for doing so?

asarkar commented 4 years ago

both the server and the client side support none value

I just think it’s good to be consistent, and in my case, both are needed.

asarkar commented 4 years ago

deprecate the port=-1 feature. Any suggestion for doing so?

Sorry, I didn't clearly answer this question before. I agree deprecating it is a good idea; you can deprecate in 2.11.0 and output a warning. However, according to semver, public methods/features can only be removed in major version changes, which for this library would be v3, so you may be stuck with it for a while. See https://github.com/semver/semver.org/issues/24 and https://github.com/semver/semver/commit/e0f985a7266ac2fdbc4076f159e1008926c6fe1c.

asarkar commented 4 years ago

Looking at how other frameworks are supporting gRPC, seems Micronaut can disable the server using grpc.server.enabled=false. It doesn’t auto configure clients, so there’s no equivalent client configuration.

https://micronaut-projects.github.io/micronaut-grpc/snapshot/guide/index.html

If you implement this, the client can be disable similarly, using grpc.client.enabled=false, or for specific ones, grpc.client.some-service.enabled=false. This is probably going to be the most intuitive and better than magic prefixes.