Skip to content
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

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

Open
asarkar opened this issue Aug 30, 2020 · 10 comments · May be fixed by #448
Open

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

asarkar opened this issue Aug 30, 2020 · 10 comments · May be fixed by #448
Assignees
Labels
enhancement A feature request or improvement
Milestone

Comments

@asarkar
Copy link

asarkar commented Aug 30, 2020

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.

@asarkar asarkar added the enhancement A feature request or improvement label Aug 30, 2020
@ST-DDT
Copy link
Collaborator

ST-DDT commented Aug 30, 2020

Let's check whether I got you right.

  • client: address is none:... -> that stub wont be created/injected (null)
  • server: address is none -> server is not created

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
Copy link
Author

asarkar commented Aug 30, 2020

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
Copy link
Collaborator

ST-DDT commented Aug 30, 2020

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.

@ST-DDT ST-DDT added this to the 2.11.0 milestone Aug 30, 2020
@asarkar
Copy link
Author

asarkar commented Aug 30, 2020

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
Copy link
Collaborator

ST-DDT commented Aug 30, 2020

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
Copy link
Author

asarkar commented Aug 30, 2020

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
Copy link
Collaborator

ST-DDT commented Aug 30, 2020

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
Copy link
Author

asarkar commented Aug 30, 2020

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
Copy link
Author

asarkar commented Aug 30, 2020

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 semver/semver.org#24 and semver/semver@e0f985a.

@asarkar
Copy link
Author

asarkar commented Aug 31, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature request or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants