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

reflection: accept interface instead of grpc.Server struct in Register() #4340

Merged
merged 3 commits into from Apr 27, 2021
Merged

reflection: accept interface instead of grpc.Server struct in Register() #4340

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
855c2fa
add reflection.RegisterWithRegistrar
jhump Apr 14, 2021
80bfb8a
have other entry point accept a map, not a function
jhump Apr 14, 2021
77b94ac
use alternate interface; change signature of Register
jhump Apr 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions reflection/serverreflection.go
View file
Open in desktop
Expand Up @@ -56,7 +56,7 @@ import (

type serverReflectionServer struct {
rpb.UnimplementedServerReflectionServer
s *grpc.Server
queryServices func() map[string]grpc.ServiceInfo

initSymbols sync.Once
serviceNames []string
Expand All @@ -65,8 +65,15 @@ type serverReflectionServer struct {

// Register registers the server reflection service on the given gRPC server.
func Register(s *grpc.Server) {
rpb.RegisterServerReflectionServer(s, &serverReflectionServer{
s: s,
RegisterWithRegistrar(s, s.GetServiceInfo)
}

// RegisterWithRegistrar registers the server reflection service with the given registrar.
// The given function is invoked to determine the services that will be exposed. It is not
// invoked until the service impl is actually used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not invoked until the service impl is actually used.

Is this important for you?

I'm thinking about another option, to take a map instead of func() map.
It has the advantage that the result will be fixed (instead of dynamic when queryServices() is called). But it also loses some flexibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine. I did it this way only so it worked more like the other version, which let you provide the server before it was done accumulating all of the registered services.

Under-the-hood, I think it will need the function and delayed query, just to support the other entry-point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. Let me know what you think.

Copy link
Contributor

@menghanl menghanl Apr 15, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, right. Sorry, I forgot that *grpc.Server.GetServiceInfo() needs to be delayed. Then maybe what you had was better. Or, option 3 is to make it another interface.

But let's wait for @dfawley's opinion on this (he's out this week, and will be back next Monday).

Copy link
Contributor

@menghanl menghanl Apr 15, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in your use case, are you able to generate a complete list of map[string]grpc.ServiceInfo before all the services are registered?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm throwing yet another idea here:

The interface method can be GetServiceInfo(string) grpc.ServiceInfo.

This gives the implementation a chance to intercept the lookup.
While in the old way, the server cannot intercept the map lookup (they cannot overload []).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of intercepting map lookup, the simpler interface I described allows a custom impl to intercept the map retrieval -- like modify the map or produce a different/derivative one. Also, your proposed method alone is insufficient -- how does the caller know what string names to use? You'd also have to have a method that provides the list of services exposed.

Finally, if it has the same name (GetServiceInfo) then *grpc.Server cannot implement it without a backwards incompatible change. So we'd also have to add an adapter type in this package, just to adapt the server to whatever new interface is used.

I personally think sticking with the interface already exposed by *grpc.Server would be best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@menghanl or @dfawley, any other thoughts about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, didn't see this thread in the "files changed" view.

I agree with your last comment, @jhump. @easwars what are the benefits of adding this to the grpc package? That seems counter to Go design ideals - the only reason ServiceRegistrar is there is because we didn't want it to be in every single generated code package for...reasons... I disagreed with them, but went along with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got confused with supporting the xds.GRPCServer and got it wrong here. Yes, no good reason to define it in the grpc package.

func RegisterWithRegistrar(r grpc.ServiceRegistrar, queryServices func() map[string]grpc.ServiceInfo) {
rpb.RegisterServerReflectionServer(r, &serverReflectionServer{
queryServices: queryServices,
})
}

Expand All @@ -80,7 +87,7 @@ type protoMessage interface {

func (s *serverReflectionServer) getSymbols() (svcNames []string, symbolIndex map[string]*dpb.FileDescriptorProto) {
s.initSymbols.Do(func() {
serviceInfo := s.s.GetServiceInfo()
serviceInfo := s.queryServices()

s.symbols = map[string]*dpb.FileDescriptorProto{}
s.serviceNames = make([]string, 0, len(serviceInfo))
Expand Down