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

Reconsider the name/usage of io.quarkus.grpc.runtime.annotations.GrpcService #16872

Closed
mkouba opened this issue Apr 28, 2021 · 8 comments · Fixed by #16908
Closed

Reconsider the name/usage of io.quarkus.grpc.runtime.annotations.GrpcService #16872

mkouba opened this issue Apr 28, 2021 · 8 comments · Fixed by #16908
Assignees
Labels
area/grpc gRPC kind/enhancement New feature or request
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Apr 28, 2021

This annotation is currently a CDI qualifier and is used to inject a gRPC stub. On the server side a gRPC service should be annotated with @javax.inject.Singleton and extend the appropriate service definition (aka the abstract ImplBase class).

I think that it would make sense to rename the qualifier to something like @GrpcClient (aligned with MP rest client) or @GrpcStub. And use the @GrpcService to mark a gRPC service (i.e. impl of io.grpc.BindableService). Note that a user does not care about the fact that his gRPC service is a bean, similarly if you create a JAX-RS endpoint you don't need to define the CDI scope either.

Moreover, the annotation should not live in io.quarkus.grpc.runtime.annotations which is by convention not part of the public API.

So my proposal is to:

  1. Move io.quarkus.grpc.runtime.annotations.GrpcService to io.quarkus.grpc.GrpcClient or io.quarkus.grpc.GrpcStub
  2. Introduce io.quarkus.grpc.GrpcService that would be used to mark a gRPC service class (instead of @Singleton)
@mkouba mkouba added kind/enhancement New feature or request area/grpc gRPC labels Apr 28, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 28, 2021

/cc @cescoffier, @michalszynkiewicz

@michalszynkiewicz
Copy link
Member

@cescoffier wdyt? If we're going to do it better do it sooner than later and changing the major number in the version is a good occasion

@mkouba
Copy link
Contributor Author

mkouba commented Apr 28, 2021

Of course, this would be a breaking change...

@mkouba
Copy link
Contributor Author

mkouba commented Apr 28, 2021

We could also make io.quarkus.grpc.runtime.annotations.GrpcService.value() optional and derive the value from the injection point by default (i.e. a field or param name).

@cescoffier
Copy link
Member

Move io.quarkus.grpc.runtime.annotations.GrpcService to io.quarkus.grpc.GrpcClient or io.quarkus.grpc.GrpcStub

I would go for client. Stub is reminding me RMI and Corba.

Introduce io.quarkus.grpc.GrpcService that would be used to mark a gRPC service class (instead of @singleton)

+1 - It would be better too as we can force it to be singleton (it has to be, because it cannot be proxied)

I believe we can handle the change without too much breakage. But if we have to, it's the right time.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 28, 2021

I would go for client. Stub is reminding me RMI and Corba.

No problem. I thought it would make sense because the generated class that is injected has the Stub suffix ;-).

I believe we can handle the change without too much breakage.

Any ideas how? We could mark the annotation as deprecated and pollute the code with special branches for deprecated parts but I don't think it's worth the effort.

@mkouba mkouba self-assigned this Apr 28, 2021
@cescoffier
Copy link
Member

Yes, that's what I was thinking, but if it gets too ugly, we should just break.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 28, 2021

Not only it's ugly but we'll end up with two @GrpcService annotations on the class path... which is not good.

mkouba added a commit to mkouba/quarkus that referenced this issue Apr 29, 2021
- rename io.quarkus.grpc.runtime.annotations.GrpcService to
io.quarkus.grpc.GrpcClient
- the relevant literal class is now a static nested class
- GrpcClient.value() is optional and the service name can be derived
from the annotated element
- introduce the io.quarkus.grpc.GrpcService CDI stereotype that can be
used to mark a gRPC service class (instead of @singleton)
- update docs and tests
- resolves quarkusio#16872
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/grpc gRPC kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants