gRPC transport initial work with crossdock testing #863

bufdev · 2017-03-30T15:24:49Z

Done so far:

end-to-end working including testing with a client and server using protobuf
end-to-end working including testing with a generated grpc-go client talking to a yarpc server using protobuf
end-to-end working with thrift and json including testing in examples
oneway working
crossdock testing for protobuf, and for grpc with all encodings (only on go), and with oneway testing, except for a bug with context propagation, there's an issue that the context gets cancelled after the oneway call, and i think this issue could affect all oneway implementations, but other implementations get around it by effectively creating a new context, see handlers.go

To do:

couple cleanup items
more testing (the testing in encoding/x/protobuf/testing actually covers about 76% of transport/x/grpc but ya)
examples with thrift, json, etc
use peer.Chooser etc
get all keys for transport.Request set from headers

Generally I want to get most of this in, and then iterate from there. This is a start, we can refine as we go, I want to get the basics committed to x/ and then I can add stuff with more granularity.

bufdev · 2017-04-11T00:44:28Z

Just for the record, I don't agree with a couple of the filed issues/proposed solutions, and I think a couple of them are bigger than just gRPC (ie they really apply to all transports, and are larger cleanup tasks across the transport package), but we can discuss heh :)

…

On Tue, Apr 11, 2017 at 2:39 AM Kris Kowal ***@***.***> wrote: @peter-edge <https://github.com/peter-edge> @willhug <https://github.com/willhug> For proxies, I imagine we’ll have to support an HTTP/2 shunt or bypass. We have a proxy in production that does this for HTTP REST services and this will likely be more of the same. We will need to explore first class support #910 <#910> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#863 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECGvN4yV9TcQs0otwEU919_d8zLodhAks5rusvHgaJpZM4MuhcK> .

willhug

Alright, I've gone through pretty deep on most of the code. For now I'm going to request changes, but I'll take another look tomorrow morning and approve after that.

Note I've made a few comments about linking "TODOs" to tasks/issues where we can follow-up. This will give people who stumble upon these TODOs a place to look for follow-ups/issues with the TODOs.

willhug · 2017-04-08T00:28:25Z

transport/x/grpc/handlers.go

+	if err := request.ValidateUnaryContext(ctx); err != nil {
+		return nil, err
+	}
+	protobuf.SetRawResponse(transportRequest.Headers)


we seem to have pretty tight coupling between the GRPC transport and the Protobuf encoding. Is it possible for us to avoid this? It reduces the flexibility of both the transport and the encoding.

willhug · 2017-04-11T00:41:38Z

transport/x/grpc/codec.go

+
+func (customCodec) String() string {
+	// TODO: faking this as proto to be compatible with existing grpc clients
+	return "proto"


for this todo, can we link to this task: #911

willhug · 2017-04-11T00:42:22Z

transport/x/grpc/doc.go

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+// Package grpc implements the grpc transport.


We should elaborate here a bit more about how the structure, how this is experimental, and should not be used (yet).

I've added a comment about the experimental status, but note that existing x/ packages are inconsistent on this topic. This comment I know is a nit, but in the spirit of tracking issues, I've added #918 to track this

willhug · 2017-04-11T00:44:58Z

transport/x/grpc/handler.go

+)
+
+type handler struct {
+	procedureServiceName string


maybe we should prefix these with their "origin"

yarpcServiceName grpcServiceName grpcMethodName

I'm keep having to remind myself the difference between the three strings.

willhug · 2017-04-11T00:47:41Z

transport/x/grpc/handler.go

+	}
+}
+
+func (h *handler) handle(


can we add a comment about how the GRPC handler does not put context first, and therefore has invalid lint?

willhug · 2017-04-11T02:45:58Z

internal/crossdock/client/dispatcher/dispatcher.go

+// CreateTransport creates an RPC from the given parameters or fails the whole behavior.
+//
+// If trans is non-empty, this will be used instead of the behavior transport.
+func CreateTransport(t crossdock.T, trans string) *yarpc.Dispatcher {


CreateDispatcherForTransport?

Transport is already a top-level object, so it's a bit confusing that a "CreateTransport" function returns a dispatcher.

Ya for sure, updated

willhug · 2017-04-11T02:50:29Z

internal/crossdock/server/oneway/server.go

+// useGrpc checks to see if a grpc server is expected to be
+// available
+func useGrpc() bool {
+	return os.Getenv("GRPC") == "enabled"


is this envVar limited to just the oneway tests? And, if so, should we make the envVar more explicit? (Additionally, I'm not sure how I feel about gating this test based on whether an environment variable is set, I could see some weird "i don't see the error" cases shrug)

This is something I don't like either, but it's how the existing setup is. I agree with you, I think this could lead to misleading errors, and that we should fail if a transport is not able to come up, without having any conditionals in environment variables. I'm making a tracking issue for this as a cleanup task #916.

willhug · 2017-04-11T02:52:21Z

internal/crossdock/server/yarpc/server.go

@@ -45,13 +48,18 @@ func Start() {
 	if err != nil {
 		log.Panicf("failed to build ChannelTransport: %v", err)
 	}
+	grpcListener, err := net.Listen("tcp", ":8089")


can we share tcp connections in grpc? If not, should we be exposing it?

Will comment more on this later, but this is re #866 and #897

willhug · 2017-04-11T02:54:13Z

internal/examples/json-keyvalue/server/main.go

@@ -102,6 +104,12 @@ func do() error {
 			return err
 		}
 		inbound = tchannelTransport.NewInbound()
+	case "grpc":
+		listener, err := net.Listen("tcp", "127.0.0.1:24038")


do we need the 127.0.0.1? Not blocking, just curious

This is a recommendation that Prashant had previously. I don't actually think it makes a difference, but I'm trying to do it for new connections I make.

willhug · 2017-04-11T02:57:25Z

internal/protoplugin/protoplugin.go

@@ -159,6 +159,16 @@ type Service struct {
 	Methods []*Method
 }

+// FQSN returns a fully qualified service name of this service.


can we add that to the comment here? Just for clarity in the code when someone else comes along.

willhug

whoops, requesting changes

bufdev · 2017-04-11T15:32:20Z

I've responded to all comments, and I've tried to make sure I have linked to current discussions and/or issues where I should, I hope this helps

willhug · 2017-04-11T20:30:22Z

transport/x/grpc/headers.go

+	caller, callerErr := getFromMetadata(md, callerHeader)
+	request.Caller = caller
+	encoding, encodingErr := getFromMetadata(md, encodingHeader)
+	request.Encoding = transport.Encoding(encoding)


does this cast work if the encoding is null/empty?

Yea, the cast is fine regardless

willhug

That was a doozy, close enough

kriskowal · 2017-04-12T03:13:46Z

transport/x/grpc/options.go

+	return o.tracer
+}
+
+func defaultOnewayErrorHandler(err error) {


shrug emoji

HelloGrayson

This is really exciting.

We'll want to follow up with Crossdock tests that verify:

a gRPC client can call a YARPC server
a YARPC client can call a gRPC server

HelloGrayson · 2017-04-12T18:19:24Z

docker-compose.yml

        environment:
            - REDIS=enabled
            - CHERAMI=enabled
+            - GRPC=enabled


I understand why Redis and Cherami need to be toggleable, but why GRPC?

I was just following the pattern, I will remove, also note #916

HelloGrayson · 2017-04-12T18:20:27Z

glide.yaml

@@ -10,6 +10,10 @@ import:
  version: ~0.4
 - package: github.com/golang/mock
  version: master
+- package: github.com/grpc-ecosystem/grpc-opentracing
+  version: master


No semver here?

grpc-opentracing isn't doing releases unfortunately :( we need to evaluate grpc-opentracing more anyways before grpc can get out of the x/ package

HelloGrayson · 2017-04-12T18:21:39Z

internal/crossdock/client/echo/protobuf.go

+}
+
+// ProtobufTransport implements the 'protobuf' behavior for the given transport or behavior transport.
+func ProtobufTransport(t crossdock.T, transport string) {


Not wild about the naming of this func... think you can introduce a verb in here?

Changed to ProtobufForTransport, also updated this for json/raw/thrift

bufdev · 2017-04-12T19:02:15Z

@breerly ya I want to add those crossdock tests as well, also in one other non-common language if possible, filed #934

HelloGrayson

Great start - it's exciting to start iterating from here with the goal of getting gRPC and protobuf out of x/ and into peoples hands.

bufdev added 30 commits March 27, 2017 15:55

Initial grpc code

fea2579

More grpc work

8ccec60

More grpc work

840241a

Copy in noopCodec from grpc branch

c0c9838

More grpc work

f20daac

More grpc work

4af2e29

More grpc work

9cb61f4

More grpc work

87d286f

Merge branch 'dev' into pgrpc

0c58468

Merge branch 'dev' into pgrpc

2613453

Fix golint

8fdfd51

Merge branch 'dev' into pgrpc

328a2cb

More grpc work

82a0b02

Custom codec

48c4329

Merge branch 'dev' into pgrpc

13b13f2

Generate grpc for protobuf example

77acfec

Add TransportTypeGRPC

904c1f2

Add grpc ClientConn to testutils

30740b3

Update protobuf example input

47cefc5

Fix

d3937f3

FQSN

54735d0

fix

d1803f7

fix

3e38dfe

Allow switching example to grpc client

45bf89c

more logging

bec7415

More work

1589afa

Transport writer

2b77918

Merge

07f2c92

Fix golint

17d99d5

Commit

d763d72

willhug reviewed Apr 11, 2017

View reviewed changes

willhug suggested changes Apr 11, 2017

View reviewed changes

bufdev added 3 commits April 11, 2017 13:30

Merge

1c500d3

Add comment about response headers

ac5cd33

Rename CreateTransport to CreateDispatcherForTransport

9e8d197

bufdev mentioned this pull request Apr 11, 2017

Remove control of which inbounds are created on crossdock using environment variables #916

Closed

Comment about fully-qualified protobuf names

210af50

bufdev mentioned this pull request Apr 11, 2017

Better understand and handle the grpc.ClientConn object #917

Closed

Link to issue about grpc codec

3d9f351

bufdev mentioned this pull request Apr 11, 2017

Nit: better documentation of experimental status on x/ packages? #918

Closed

Add comment on grpc package

fb9f0e6

bufdev mentioned this pull request Apr 11, 2017

Always have a link to a GitHub issue for TODOS? #919

Closed

bufdev added 3 commits April 11, 2017 15:32

More fixes

5529d7a

Fix issues

e0b13f4

Fixes

2cc2444

willhug reviewed Apr 11, 2017

View reviewed changes

willhug approved these changes Apr 11, 2017

View reviewed changes

kriskowal approved these changes Apr 12, 2017

View reviewed changes

transport/x/grpc/options.go

return o.tracer

}

func defaultOnewayErrorHandler(err error) {

Copy link

Contributor

kriskowal Apr 12, 2017

Choose a reason for hiding this comment

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

shrug emoji

HelloGrayson reviewed Apr 12, 2017

View reviewed changes

bufdev added 3 commits April 12, 2017 20:56

Merge

725b8cb

Remove GRPC variable that controls GRPC inbound in crossdock

c3d649e

Update FooTransport to FooForTransport in crossdock client

9980462

HelloGrayson approved these changes Apr 13, 2017

View reviewed changes

bufdev added 2 commits April 18, 2017 11:34

Merge

00d8552

Remove grpc log code

f3ffdef

bufdev merged commit 36f9f63 into dev Apr 18, 2017

bufdev deleted the pgrpc branch April 18, 2017 23:10

gRPC transport initial work with crossdock testing #863

gRPC transport initial work with crossdock testing #863

Conversation

bufdev commented Mar 30, 2017 • edited Loading

bufdev commented Apr 11, 2017 via email

willhug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willhug left a comment

Choose a reason for hiding this comment

bufdev commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willhug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HelloGrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufdev commented Apr 12, 2017

HelloGrayson left a comment

Choose a reason for hiding this comment

bufdev commented Mar 30, 2017 •

edited

Loading