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.

Sign up for GitHub

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

Jump to bottom

Cannot build with grpc-java:1.30.0 due to ProtoReflectionService change #2806

Closed
okue opened this issue Jun 18, 2020 · 3 comments · Fixed by #2814
Closed

Cannot build with grpc-java:1.30.0 due to ProtoReflectionService change #2806

okue opened this issue Jun 18, 2020 · 3 comments · Fixed by #2814
Labels
dependencies
Milestone
0.99.7

Comments

@okue
Copy link
Member

okue commented Jun 18, 2020 •
edited

Since ProtoReflectionService of grpc-java:1.30.0 is not a InternalNotifyOnServerBuild, armeria have to changes here.

https://github.com/line/armeria/blob/armeria-0.99.6/grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java#L271-L275

(note: notifyOnBuild passes a list of ServerServiceDefinition)

But, changes seem not so obvious because ProtoReflectionService comes to get io.grpc.Server from io.grpc.Context.

https://github.com/grpc/grpc-java/blob/v1.30.0/services/src/main/java/io/grpc/protobuf/services/ProtoReflectionService.java#L89

related:
@anuraaga
Copy link
Collaborator

I think we could add an interceptor in front of the reflection service that adds our fake Server to context. Pretty hacky though probably good to file an issue upstream to make the reflection service configurable so it can be used outside of grpc-java

@ikhoon
Copy link
Contributor

ikhoon commented Jun 18, 2020

@anuraaga Are you working or considering on this? Otherwise, I will take a look at this.

@anuraaga
Copy link
Collaborator

Don't think I'll be able to get to it so all yours :) Please don't forget to start the upstream discussion

@ikhoon ikhoon mentioned this issue Jun 18, 2020
Open
ikhoon added a commit to ikhoon/armeria that referenced this issue Jun 18, 2020
@ikhoon
Update gRPC-Java to 1.30.0
559cb05 
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
@ikhoon
Update gRPC-Java to 1.30.0
05be143 
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
@ikhoon ikhoon mentioned this issue Jun 18, 2020
Merged
trustin pushed a commit that referenced this issue Jun 19, 2020
@ikhoon
Update gRPC-Java to 1.30.0 (#2814)
9913a3a 
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
@trustin trustin added this to the 0.99.7 milestone Jun 19, 2020
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
@ikhoon
Update gRPC-Java to 1.30.0 (line#2814)
0372b54 
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
Assignees
No one assigned
Labels
dependencies
Projects
None yet
Milestone
0.99.7
Development

Successfully merging a pull request may close this issue.

Update gRPC-Java to 1.30.0 ikhoon/armeria
4 participants
@trustin @anuraaga @ikhoon @okue