-
Notifications
You must be signed in to change notification settings - Fork 437
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
Add a Connection Broker for GRPC plugins #52
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, great work and clever implementation. Also happens to align well with the net/rpc style which is a huge plus.
I may be missing something, but where is newGRPCBroker
and such functions? Curious what their lifecycle is like and I can't seem to find them.
@mitchellh The In that file is the implementation of the GRPCBroker as well as the server/client used for the gRPC stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew, took a bit to work through this but this looks good.
It'd be great to see docs on how this works put into the docs/
folder. That is a recent addition (with initial grpc) so its less in depth but I'd like to start making sure we update that so that the whole system is easier to understand going forward.
Get(key string) (int64, error) | ||
} | ||
|
||
// This is the implementation of plugin.Plugin so we can serve/consume this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be plugin.GRPCPlugin instead of plugin.Plugin.
Nevermind, this embeds plugin.NetRPCUnsupportedPlugin which is the plugin.Plugin implementation.
grpc_broker.go
Outdated
return | ||
case <-s.quit: | ||
return | ||
case s := <-s.send: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, but perhaps rename s
into something else, since s
initially refers to *gRPCBrokerServer
, but after the assignment it's a *sendErr
. Even though there's not conflict in overriding s
in this case, it makes it somewhat harder to read. Same for StartStream
on the client side.
This adds a broker object that can serve and dial connections via connection ids. It uses a streaming gRPC service to communicate the connection details between a client and a plugin. Each
AcceptAndServe
call opens a new listener socket and sends the connection info down the stream to the dialer. Since a new connection is opened every call, these calls should be used sparingly. Multiple gRPC server implementations can be registered to a singleAcceptAndServe
call.Plugins written in another language will, optionally, have to implement the gRPC server/client for the broker, but we provide the proto file for this definition.
Unfortunately there is a slight API breaking change with the
GRPCPlugin
interface. TheGRPCServer
andGRPCClient
now accept aGRPCBroker
as the first parameter. Open to ideas on how to make this BC.