Clean up code flow for gRPC server setup and server registration #6442
Conversation
|
Added @vadimberezniker too so this gets a second set of eyes |
| func RegisterInternalGRPCSServer(env *real_environment.RealEnv, regServices RegisterServices) error { | ||
| if *internalGRPCSPort == 0 { | ||
| return nil | ||
| func NewBuilder(env environment.Env, port int, ssl bool) (*Builder, error) { |
There was a problem hiding this comment.
I was a little thrown off by the naming as this is not a "builder" in the conventional sense. What do you think instead of returning a *Builder you return (*grpc.Server server, func() error, error) and get rid of the *Builder struct altogether? You could also rename NewBuilder to New so at the call site the call becomes s, start, err := grpc_server.New(...).
There was a problem hiding this comment.
Yeah, not a great name. In a previous iteration it was more builder-y. I'd prefer to keep the struct because in the future I think there'll be more configuration options added and I think it's a bit more clear that passing callbacks around, so I renamed this to GRPCServer.
There was a problem hiding this comment.
Do you have a specific example in mind of what you think will be added to the struct in the future? I'm not a big fan of making speculative changes for something that may not ever come to be. The struct can always be added in the future, as needed if more complexity is required.
As far as returning a function, it's not that uncommon to do in Go code. I don't see a big difference between asking the user to call Start on the struct or calling the returned start function.
There was a problem hiding this comment.
I'm going to add a way to add additional RPC interceptors for cache proxy auth.
I know it's not uncommon to return callbacks in go and it's effectively the same thing, but I always find passing functions around more confusing than working with an object. I can change it if you'd really like but part of my motivation with this change was to remove the closures.
server/libmain/libmain.go
Outdated
| @@ -218,14 +218,65 @@ func GetConfiguredEnvironmentOrDie(healthChecker *healthcheck.HealthChecker, app | |||
| return realEnv | |||
| } | |||
|
|
|||
| func registerInternalGRPCServices(grpcServer *grpc.Server, env *real_environment.RealEnv) { | |||
| func registerInternalGRPCServices(env *real_environment.RealEnv) error { | |||
There was a problem hiding this comment.
I would call these methods something along the lines of startInternalGRPCServers rather than register... as you are not only registering gRPC services here but also starting the actual gRPC server.
no more closures
Related issues: N/A