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

Consider configuring ProtoReflectionService to use it without internal APIs #7138

Open
ikhoon opened this issue Jun 18, 2020 · 5 comments
Open
Milestone

Comments

@ikhoon
Copy link

ikhoon commented Jun 18, 2020

Due to the popularity of gRPC, some frameworks or libraries have an integration layer for gRPC.

Especially, Armeria has its own server/client implementations to serve/call a gRPC stub.
ProtoReflectionService is using the internal reference of a gRPC-Java server instance since 1.30.0 #6967.
That means a stub running without gRPC-Java server could not use ProtoReflectionService. line/armeria#2806

Armeria injects ServerServiceDefinitions to ProtoReflectionService using notifyOnBuild() hook.
https://github.com/line/armeria/blob/armeria-0.99.6/grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java#L271-L275
If ProtoReflectionService supports better ways to build ProtoReflectionService,
it could be a good extension point for gRPC-Java ecosystem.

ikhoon added a commit to ikhoon/armeria that referenced this issue Jun 18, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 line#2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Motivations:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
ikhoon added a commit to ikhoon/armeria that referenced this issue Jun 18, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 line#2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Motivations:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
Fixes line#2806
@voidzcy
Copy link
Contributor

voidzcy commented Jun 18, 2020

The server reflection protocol in gRPC was designed

as an optional extension for servers to assist clients in runtime construction of requests without having stub information precompiled into the client.

So its primary usage is to expose services running on a gRPC server.

Armeria's usage of ProtoReflectionService is a pretty hacky way that takes advantage of Server's interface to expose a list of custom service definitions at runtime via the the reflection service. It is not what we intended.

You could design/define your own gRPC service to expose other services at runtime that does not depend on Server interface. A.k.a., a simple version of ProtoReflectionService (btw ProtoReflectionService itself is just a normal gRPC service and it's an optional layer installed on top of the a gRPC server).

@ikhoon
Copy link
Author

ikhoon commented Jun 18, 2020

You could design/define your own gRPC service to expose other services at runtime that does not depend on Server interface. A.k.a., a simple version of ProtoReflectionService

We don't really need ProtoReflectionService itself. A simple version sounds good to me. 👍

@ejona86
Copy link
Member

ejona86 commented Jun 18, 2020

I'd actually expect Armeria to have a reasonably good time just returning the existing Server shim from ServerCall instead of passing it to notifyOnBuild(). That's not great, but the lifecycle methods are of dubious use from the ServerCall API. Note that it is also possible to do that just for the reflection service via an interceptor.

It might be fair to split the non-lifecycle methods out of Server, to have a "managed" vs "unmanaged" split like we have for Channel. But it seems that may be an overkill.

@ikhoon
Copy link
Author

ikhoon commented Jun 18, 2020

Note that it is also possible to do that just for the reflection service via an interceptor.

As you said, I implemented an intercepter that injects a dummy server to the current gRPC context. It is a workaround but a hacky way. It would be nice to support an API not to depend on gRPC internal implementations. :-)

@ejona86
Copy link
Member

ejona86 commented Jun 18, 2020

Oh, I forgot we swapped to the internal Context key. Earlier in the iteration we had a ServerCall.getServer() API. We were having trouble settling on the replacement API and it seemed better to do the Context hack as an internal API to replace the previous broken internal API instead of delaying any longer.

trustin pushed a commit to line/armeria that referenced this issue Jun 19, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 #2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Modifications:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
Fixes #2806
@ejona86 ejona86 changed the title Consider configuring ProtoReflectionService to use it without gRPC-Java server Consider configuring ProtoReflectionService to use it without internal APIs Jul 1, 2020
@ejona86 ejona86 added this to the Next milestone Jul 1, 2020
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
Motivation:

gRPC-Java [1.30.0](https://github.com/grpc/grpc-java/releases/tag/v1.30.0) has been released
We need a workaround for `ProtoReflectionService`. Because it does not implement `InternalNotifyOnServerBuild` anymore.
See grpc/grpc-java#6967 line#2806
It is a temporary workaround until upstream handles grpc/grpc-java#7138.

Modifications:

- Upgrade gRPC to 1.30.0 from 1.29.0
- Add ProtoReflectionServiceInterceptor that injects dummy server to
  gRPC context.

Result:

You can run gRPC 1.30.0 with Armeria server.
Fixes line#2806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants