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

Server reflection service #651

Closed
ejona86 opened this issue Jul 21, 2015 · 13 comments
Closed

Server reflection service #651

ejona86 opened this issue Jul 21, 2015 · 13 comments
Assignees
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented Jul 21, 2015

No description provided.

@ejona86 ejona86 added this to the 0.8.0 milestone Jul 21, 2015
@zhangkun83
Copy link
Contributor

The goal is to be able to propagate any reflection information, e.g., type names of the request and the response, from the generated code to the runtime. That information is specific to the marshalling system, e.g., Protobuf. I am proposing this:

  1. Extend MethodDescriptor to carry marshaller-specific reflection information. For example, we will have ProtobufMethodDescriptor that carries maybe ServiceDescriptorProto, MethodDescriptorProto etc. The generated code will create ProtobufMethodDescriptor instead of the base MethodDescriptor, and pass it to ServerMethodDefinition
  2. We create a new interface Reflectable which provides Collection<MethodDescriptor> getAllMethods(). A server is reflectable only if its HandlerRegistry implements this interface. (A proxy server may not be reflectable, so it doesn't necessarily implement this interface. It should forward reflection requests to the real server.).
  3. The server reflection service is marshaller-specific. There is a Protobuf reflection service that works only with services that use Protobuf. It expects ProtobufMethodDescriptor from getAllMethods().

@ejona86
Copy link
Member Author

ejona86 commented Aug 6, 2015

That general approach SGTM, and is additive-only. We may need some other plumbing to get the HandlerRegistry to the reflection service, but that also seems it could be additive.

@nmittler
Copy link
Member

@zhangkun83 these changes would be server-side only, correct? We probably don't want this baggage on the client-side.

@ejona86
Copy link
Member Author

ejona86 commented Aug 10, 2015

I could see the extra information being useful on client-side, too, and it doesn't seem that it adds almost any memory baggage or the like.

@zhangkun83
Copy link
Contributor

Agreed with Eric. The extended MethodDescriptor is already in the generated code, whether or not you pass it to the client-side runtime.

@nmittler
Copy link
Member

As it stands now, if you have an instance of your request proto is a ServiceDescriptorProto instance created automatically? Or is it created lazily?

If it is automatic, then I agree there's no problem. If it's lazy, then we should consider how much extra memory is required to hold a ServiceDescriptorProto in-mempory when the client may very well not use it.

@zhangkun83
Copy link
Contributor

The descriptors are created automatically in the static block of the generated message class.

@nmittler
Copy link
Member

@zhangkun83 ok, in that case SGTM :)

@zhangkun83 zhangkun83 removed this from the 0.8.0 milestone Aug 12, 2015
@ejona86 ejona86 added this to the 1.0 milestone Nov 6, 2015
@zhangkun83 zhangkun83 changed the title Determine API changes necessary for server reflection service Server reflection service Nov 24, 2015
@ejona86 ejona86 modified the milestones: 1.1, 1.0 Apr 19, 2016
@mwitkow
Copy link

mwitkow commented Jul 1, 2016

Since like support for server side reflection landed in Go: https://github.com/grpc/grpc-go/tree/master/reflection. @ejona86, any update here?

@carl-mastrangelo
Copy link
Contributor

Reflection was put on hold until after GA.

@mwitkow
Copy link

mwitkow commented Jul 4, 2016

@carl-mastrangelo that's ok :) Do you have a link somewhere to the GA/post-GA roadmap somewhere? :) Which quarter do you think it may happen in?

@buchgr
Copy link
Collaborator

buchgr commented Jul 4, 2016

@ejona86
Copy link
Member Author

ejona86 commented Jan 17, 2017

This is in #2360, #2509, and #2532.

@ejona86 ejona86 closed this as completed Jan 17, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Sep 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants