cmd/protoc-gen-go-grpc: rework service registration #3828
Conversation
There was a problem hiding this comment.
Reviewed 64 of 65 files at r1.
Reviewable status: 64 of 65 files reviewed, 9 unresolved discussions (waiting on @dfawley and @menghanl)
go.mod, line 15 at r1 (raw file):
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a google.golang.org/genproto v0.0.0-20200624020401-64a14ca9d1ad google.golang.org/grpc/examples v0.0.0-20200811135751-6aaac03d175a // indirect
Why? What changed caused this?
server.go, line 557 at r1 (raw file):
// RegisterService registers a service and its implementation to the gRPC // server. It is called from the IDL generated code. This must be called before // invoking Serve. If ss is non-nil, its type is checked to ensure it
Explain more? Say that ss being non-nil is legacy code, and it should always be nil?
balancer/grpclb/grpc_lb_v1/load_balancer.pb.go, line 715 at r1 (raw file):
} func RegisterLoadBalancerServer(s *grpc.Server, srv LoadBalancerServer) {
This is RegisterLoadBalancerServer.
The new method would be RegisterLoadBalancerService.
We can actually keep both now, right?
If we want to direct new users to do the new thing.
cmd/protoc-gen-go-grpc/grpc.go, line 32 at r1 (raw file):
const ( errorsPackage = protogen.GoImportPath("errors")
Who uses errors? The generated code doesn't seems to have it. Removed later by goimports?
examples/go.mod, line 13 at r1 (raw file):
) replace google.golang.org/grpc => ../
Remove
examples/features/proto/echo/echo_grpc.pb.go, line 84 at r1 (raw file):
func (c *echoClient) ClientStreamingEcho(ctx context.Context, opts ...grpc.CallOption) (Echo_ClientStreamingEchoClient, error) { streamDesc := &grpc.StreamDesc{
Why not keep it the old way? This creates a new struct every time the RPC is called.
If we cannot use it for registering (for the reason of the handlers?), we can use still use it for the interceptors.
And instead of index, give it a name, make it a "const".
examples/features/proto/echo/echo_grpc.pb.go, line 172 at r1 (raw file):
} func (s *EchoService) unaryEcho(srv interface{}, ctx context.Context, dec func(interface{}) error, interceptor grpc.UnaryServerInterceptor) (interface{}, error) {
s/srv/_ to make it clear that srv isn't used.
Same for the streaming calls.
examples/features/reflection/server/main.go, line 43 at r1 (raw file):
} // unaryEcho implementes echo.Echo.UnaryEcho
implements
test/grpc_testing/test.pb.go, line 7 at r1 (raw file):
import ( context "context"
Related and also not related to this file.
If we upgrade this to the newer version, stub service in e2e test is no longer necessary.
There was a problem hiding this comment.
Reviewable status: 63 of 65 files reviewed, 10 unresolved discussions (waiting on @easwars and @menghanl)
go.mod, line 15 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
Why? What changed caused this?
That's a good question.. I was able to revert these changes.
server.go, line 557 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
Explain more? Say that ss being non-nil is legacy code, and it should always be nil?
Done. I'm not going to say nobody should use it, I just don't understand why it was done this way.
balancer/grpclb/grpc_lb_v1/load_balancer.pb.go, line 715 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
This is RegisterLoadBalancer
Server.
The new method would be RegisterLoadBalancerService.
We can actually keep both now, right?
If we want to direct new users to do the new thing.
There would be other conflicting symbols (duplicate client and stream types) if we used both the old and new codegen in this package.
Unless you are recommending generating the legacy symbols in the new codegen. Or having a special way to run where it omits the client & stream symbols to work in tandem with the old codegen.
cmd/protoc-gen-go-grpc/grpc.go, line 32 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
Who uses errors? The generated code doesn't seems to have it. Removed later by goimports?
I used this when NewFooService returned an error and forgot to delete it. Done.
examples/go.mod, line 13 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
Remove
We need something to get version ServiceRegistrar before the next release includes it, and also pick up the changes to not check ss in RegisterService or else it panics.
Didn't we say this was something we wanted to do anyway?
examples/features/proto/echo/echo_grpc.pb.go, line 84 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
Why not keep it the old way? This creates a new struct every time the RPC is called.
If we cannot use it for registering (for the reason of the handlers?), we can use still use it for the interceptors.
And instead of index, give it a name, make it a "const".
Done.
examples/features/proto/echo/echo_grpc.pb.go, line 172 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
s/srv/_ to make it clear that
srvisn't used.
Same for the streaming calls.
Done.
examples/features/reflection/server/main.go, line 43 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
implements
Implemente'd!
test/grpc_testing/test.pb.go, line 7 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
Related and also not related to this file.
If we upgrade this to the newer version, stub service in e2e test is no longer necessary.
I'm thinking we should update this and the pb.go's in internal directories to the new codegen. Should we wait for a follow-up PR though to keep this smaller?
|
Per offline discussion, try keeping both old and new symbols (RegisterEchoServer and RegisterEchoService), so there is a migrating path. |
There was a problem hiding this comment.
Reviewed 8 of 9 files at r3, 10 of 17 files at r4, 21 of 22 files at r5.
Reviewable status: 64 of 65 files reviewed, 4 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)
cmd/protoc-gen-go-grpc/main.go, line 44 at r5 (raw file):
func main() { var flags flag.FlagSet migrationMode = flags.Bool("migrationMode", false, "set to generate new symbols only; requires symbols produced by legacy protoc-gen-go")
Should flags be _?
test/grpc_testing/test.pb.go, line 7 at r1 (raw file):
Previously, dfawley (Doug Fawley) wrote…
I'm thinking we should update this and the pb.go's in
internaldirectories to the new codegen. Should we wait for a follow-up PR though to keep this smaller?
SG
There was a problem hiding this comment.
Reviewable status: 64 of 65 files reviewed, 4 unresolved discussions (waiting on @easwars and @menghanl)
cmd/protoc-gen-go-grpc/main.go, line 44 at r5 (raw file):
Previously, menghanl (Menghan Li) wrote…
Should flags be
_?
No, I believe it's important we pass it to ParamFunc just below for when the library calls Parse.
test/grpc_testing/test.pb.go, line 7 at r1 (raw file):
Previously, menghanl (Menghan Li) wrote…
SG
Well, that's part of this PR anyway. ;)
There was a problem hiding this comment.
Reviewable status: 64 of 65 files reviewed, 2 unresolved discussions (waiting on @dfawley, @easwars, and @menghanl)
cmd/protoc-gen-go-grpc/main.go, line 44 at r5 (raw file):
Previously, dfawley (Doug Fawley) wrote…
No, I believe it's important we pass it to
ParamFuncjust below for when the library callsParse.
Not sure if we are talking about the same thing.
I meant, should the flag be migration_mode?
Ohh, I thought you meant we didn't need the variable ( Looks like the existing multi-word flags are underscored, not camelcase; I'll change it. |
There was a problem hiding this comment.
Reviewed 1 of 22 files at r5, 14 of 14 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @easwars)
Sadly there is a large amount of churn in generated code in this PR, due to changing over to the legacy codegen for everything besides examples to avoid breaking changes. If new methods are added to any services exposed by them, we will switch to the new version of the tool since we would be forced to break compatibility no matter what.
This change is