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鈥檒l 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 2 commits
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
22 changes: 18 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,22 @@ 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,
register(s, s.GetServiceInfo)
}

// RegisterWithRegistrar registers the server reflection service with the given registrar.
// The given map describes the services that will be exposed via reflection.
func RegisterWithRegistrar(r grpc.ServiceRegistrar, svcs map[string]grpc.ServiceInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like this instead?

type GRPCServer interface {
	grpc.ServiceRegistrar
	GetServiceInfo() map[string]ServiceInfo
}

func Register(s GRPCServer) {  // or a new method if this kind of API change is unacceptable
}

Copy link
Member Author

@jhump jhump Apr 23, 2021 •
edited

Choose a reason for hiding this comment

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

@dfawley, this is more or less what I proposed earlier: #4340 (comment)

I will push a commit to this branch that makes that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've pushed two commits.

The first one implements what you suggest above. The downside to this is that it is not strictly backwards compatible, due to the signature change for Register.

However, it has been done in past releases of this module, such as migrating things (particularly generated code) to require a grpc.ClientConnInterface or grpc.ServiceRegistrar instead of the concrete types *grpc.ClientConn and *grpc.Server.

So maybe it's okay? What do you think?

The second commit I just pushed actually makes further changes (and changes some names, in the hopes of making the API more coherent) that are strictly backwards compatible.

If you prefer the first formulation, I can rollback that last commit. Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Of the two, I prefer the former. The only affected users would be ones referring to the function by its type, and I see little reason to this. Plus it keeps the API as minimal as possible. Using an interface is mildly worrying, since it means we can't extend it in the future, but we can do type assertions (to other interfaces) or introduce a new API if we ever do need to add features. Thanks for the PR & revisions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, SGTM. I removed that last commit to take it back to the simpler form. Anything else you'd like to see, or do you think this one is good to go?

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

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

Expand All @@ -80,7 +94,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