Implement grpc-gateway Delimited interface #20983

johanbrandhorst · 2017-12-21T12:18:12Z

Implement the Delimited interface for the gogo/protobuf JSONPb marshaller. This was added in grpc-ecosystem/grpc-gateway@e4b8a93.

Fixes #20925

@tamird

cockroach-teamcity · 2017-12-21T12:18:17Z

This change is

tamird · 2017-12-21T14:48:05Z

I don't understand the point of this. Can you add a test demonstrating why this is useful? Also, shouldn't this come with an update of the grpc-gateway version?

johanbrandhorst · 2017-12-21T19:44:48Z

Sorry I should elaborate a little to explain why we might want this.

In grpc-ecosystem/grpc-gateway@e4b8a93 the grpc-gateway introduced the new Delimited interface which decides how streaming responses are delimited. This introduced a problem for non-grpc-gateway marshallers (such as the cockroachdb one) as there was no default behaviour for when marshallers didn't implement the interface until grpc-ecosystem/grpc-gateway@cde2f8f, which was not included in the latest release of the grpc-gateway (1.3.0).

So here's the scenario: you use dep, so you're using gRPC-gateway 1.3.0, and you're using the cockroachdb protoutil because you're using gogo/protobuf instead of golang/protobuf. This combination leads to no newlines between streamed JSON responses, a regression from the previously default behaviour.

This patch will prevent that, which should help others in the same situation, and adds completeness to the protoutil marshallers. It does, of course, depend on cockroachdb cutting a release with this in it as well.

I'm happy to discard this if you disagree with my reasoning, or if we just want to wait for grpc-gateway to cut a new release with the default delimiter instead (I've raised an issue already).

tamird · 2017-12-21T19:59:29Z

OK, thanks for explaining.

Actually, though we use dep, we are not using 1.3.0 - we are using this revision:grpc-ecosystem/grpc-gateway@b0be3cd (see

cockroach/Gopkg.toml

Lines 50 to 53 in 2e72997

    
           # https://github.com/grpc-ecosystem/grpc-gateway/commit/893772d 
        
           [[constraint]] 
        
             name = "github.com/grpc-ecosystem/grpc-gateway" 
        
             branch = "master"

).

I think the regression you cite is probably not an issue for us because none of our grpc-gateway RPCs are streaming. That said, if we're going to fix this, we need to have tests (similar to the ones upstream added in grpc-ecosystem/grpc-gateway@376e649) so that we can convincingly say streaming RPCs (both JSON and protobuf) work correctly. Seems like this can all be done without bumping the grpc-gateway version, which is good (because it means less work for you).

Are you interested in doing that?

tamird · 2017-12-21T20:03:52Z

pkg/util/protoutil/jsonpb_marshal.go

@@ -133,3 +133,8 @@ func (j *JSONPb) NewEncoder(w io.Writer) gwruntime.Encoder {
 		return errors.Errorf("unexpected type %T does not implement %s", v, typeProtoMessage)
 	})
 }
+
+// Delimiter for newline encoded JSON streams.


This comment should follow the convention in this file: "Delimiter implements gwruntime.Delimited." (or whatever the name of the interface is. We should also have a static assertion right above this to ensure that the interface is indeed implemented. There's a similar assertion at the top of this file:

cockroach/pkg/util/protoutil/jsonpb_marshal.go

Line 30 in edc9718

var _ gwruntime.Marshaler = (*JSONPb)(nil)

.

Please make the same changes to the protobuf marshaler as well.

The static assertion won't work unless we actually bump the grpc-gateway version, unless you want me to assert interface{ Delimiter() []byte }. I've updated the comments.

johanbrandhorst · 2017-12-21T22:10:07Z

I'll take a stab at adding the tests.

johanbrandhorst · 2017-12-21T22:51:22Z

Looks like the version of grpc-gateway you have vendored is https://github.com/grpc-ecosystem/grpc-gateway/commit/b0be3cdef0ed27e3c420795e2efbfe9c27e839cc/ which appears to be the very commit that the default \n is removed, but doesn't seem to include the logic for using the Delimited interface. This'll mean I can't test any use of the Delimited interface through the gwruntime package. I can't imagine you want me to test that Delimiter returns the correct characters, so how would you suggest I proceed with the testing?

bdarnell · 2017-12-21T22:54:53Z

pkg/util/protoutil/marshaler.go

+
+// Delimiter implements gwruntime.Delimited.
+func (*ProtoPb) Delimiter() []byte {
+	return []byte("\n")


Encoded protobufs may contain \n characters. I don't think there is any character (or sequence) that can safely be used as a delimiter here - protobufs are generally used in protocols with length-prefixed frames instead of delimiters.

That's a great point. Not sure there's too much we can do here though since it'll default to \n if we don't use it ourselves. I'll remove this again so at least it's the grpc-gateway that's wrong.

I've removed it.

tamird · 2017-12-21T22:57:03Z

Right, looks like we'd have to bump to at least grpc-ecosystem/grpc-gateway@e4b8a93 for any of this to work, and at that point we may as well bump all the way and get grpc-ecosystem/grpc-gateway@1d56520.

Either way, the test would certainly not be a direct test of the function Delimiter, but rather you'll want to do something resembling grpc-ecosystem/grpc-gateway@376e649 (as I mentioned above) - make a request, pass it through grpc-gateway stuff and the marshaler, and then pass it back through the unmarshaler (note that upstream's tests don't quite do the right thing, they directly test for the presence of a newline rather than testing that unmarshaling works - we'll want to test that unmarshaling works).

johanbrandhorst · 2017-12-21T23:08:30Z

Just to clarify; something like the commit you're pointing to should be sufficient, where we basically just run ForwardResponseStream and look at the results written to the writer? Or do you genuinely want a separate process started with custom streaming RPC methods and firing API messages over a local socket?

Also I'm not familiar with the process of updating your dependencies, is there some way we can do it atomically with this PR?

tamird · 2017-12-21T23:18:45Z

Sorry, I updated my comment after submitting it. Yes, no need to have a server running; ForwardResponseStream should suffice - however, we want to do more than the referenced commit in that we want to actually try to decode the stream, rather than just checking for newlines (or whatever delimiter).

Updating the dependencies is described in https://github.com/cockroachdb/cockroach/blob/master/build/README.md#dependencies.

johanbrandhorst · 2017-12-21T23:34:14Z

Thanks for the clarification, I'll take a stab at it, with the added test of unmarshalling as well. However, the unmarshaller itself doesn't automatically decode the stream, we'll have to split on whatever the delimiter is in the test, unless you want that kind of functionality built into the unmarshaller as well? I'd consider it a bit out of scope for this PR.

tamird · 2017-12-22T00:11:47Z

I wouldn't add that into the unmarshaler. Thinking about this a bit more, I'm no longer sure adding a test provides any value; I didn't realize that the unmarshaler doesn't actually handle streaming splitting. I think the right thing to do here is to bump grpc-gateway and add implementations and static assertions - []byte("\n") for json and nil for protobuf.

johanbrandhorst · 2017-12-22T01:10:35Z

Well, that certainly makes my job easier, thanks I'll get it in tomorrow I hope.

johanbrandhorst · 2017-12-22T11:18:53Z

I've created cockroachdb/vendored#13 to bump the grpc-gateway version.

johanbrandhorst · 2017-12-22T11:26:16Z

Made the suggested changes, will update the submodule ref pending cockroachdb/vendored#13 being merged.

benesch · 2017-12-22T16:19:23Z

We actually hold off on merging vendored until the PR here passes tests. I've pushed your PR there to a cockroachdb-owned branch, though, so it's now possible to update the submodule ref and hopefully get a green build here.

tamird · 2017-12-22T16:26:58Z

Thanks, @benesch. Isn't it possible to point to non-cockroachdb owned branches anyway, though? e.g. cockroachdb/vendored@b36bdd4

Edit: that doesn't prove anything, since you pushed the crdb owned branch. Need more coffee.

benesch · 2017-12-22T16:45:47Z

Oh, quite possibly. I don't know how GH decides what refs are available for a given remote.

…

On Fri, Dec 22, 2017 at 11:27 AM, Tamir Duberstein ***@***.*** > wrote: Thanks, @benesch <https://github.com/benesch>. Isn't it possible to point to non-cockroachdb owned branches anyway, though? e.g. ***@***.*** <cockroachdb/vendored@b36bdd4> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20983 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IHfKMazgVv_uXwlpWL9Jpy9JAcEJks5tC9hhgaJpZM4RJvx5> .

johanbrandhorst · 2017-12-22T16:50:35Z

Ok I think I've got the vendor update done correctly now as well. Please let me know if there was anything else to do.

johanbrandhorst · 2017-12-22T16:53:03Z

Gopkg.toml

  name = "github.com/grpc-ecosystem/grpc-gateway"
-  branch = "master"


I understand if you consider this to be a break with your established methods, but I didn't want to go in and mess with the Gopkg.lock file itself to achieve the desired update.

But why not leave this as is and run dep ensure -update github.com/grpc-ecosystem/grpc-gateway?

I updated this explicitly because this was the revision your specified in your comment, I misunderstood you if you meant I should just update to tip. I'll get that done.

I think I need https://github.com/cockroachdb/vendored/tree/johanbrandhorst/bump-grpc-gateway updated.

You might not. Let's see what CI says.

johanbrandhorst · 2017-12-22T18:43:05Z

Still need to update the vendor submodule pending johanbrandhorst/bump-grpc-gateway update in cockroachdb/vendored#13

tamird · 2017-12-22T18:45:09Z

Try updating the submodule ref anyway, I think it'll work.

johanbrandhorst · 2017-12-22T18:49:16Z

@tamird I don't have a ref I can update to in vendored as I don't have push permissions to vendored. I pushed to my forked repo, so someone with push permissions to vendored has to pull those changes into johanbrandhorst/bump-grpc-gateway, so I can reference that commit in this PR. I think?

tamird · 2017-12-22T18:51:02Z

Yes, I think you can - try it. Sorry, I think you can just reference your commit in johanbrandhorst/bump-grpc-gateway.

johanbrandhorst · 2017-12-22T20:02:28Z

To be clear, there are commits that I have updated that is in the fork johanbrandhorst/bump-grpc-gateway, and there are commits that were put in place by @benesch in vendored/johanbrandhorst/bump-grpc-gateway. Navigating into the submodule and running git checkout <sha from my fork> does not work, so I think the branch johanbrandhorst/bump-grpc-gateway in vendored will need to pull from my fork first, so I can reference that commit.

I have tried pushing directly to vendored/johanbrandhorst/bump-grpc-gateway and get a permissions error.

tamird · 2017-12-22T20:05:10Z

I understand, and what I'm saying is that Github does some special magic sauce to make your commits in your fork reachable from the upstream repository. Witness: cockroachdb/vendored@fc3aa43

So what I'm saying is - update the submodule ref in this PR and see what happens.

tamird · 2017-12-22T20:07:09Z

Gopkg.toml

@@ -47,7 +47,7 @@ ignored = [
  name = "github.com/go-sql-driver/mysql"
  branch = "master"

-# https://github.com/grpc-ecosystem/grpc-gateway/commit/893772d
+# https://github.com/grpc-ecosystem/grpc-gateway/pull/497


Slightly nicer if this comment references the commit (https://github.com/grpc-ecosystem/grpc-gateway/commit/8db8c1a) because it's easier to see from the github commit view when the commit has been included in a release.

johanbrandhorst · 2017-12-22T20:19:58Z

@tamird how do I update the submodule ref to another origin? Do I literally go in a add a new remote and check that out?

tamird · 2017-12-22T20:25:21Z

Yup, that should work. You can also use Github's magic without adding a remote:

git fetch origin pull/13/head:bump-grpc-gateway
git checkout bump-grpc-gateway

this will fetch and check out your PR. More: https://help.github.com/articles/checking-out-pull-requests-locally/

johanbrandhorst · 2017-12-22T20:29:29Z

Well what do you know! Sorry for my pig-headedness, I was perhaps misled by the original branch created in the vendored repo. I think I've done everything now, but I'm guessing we'll let CI decide my fate ;)

tamird · 2017-12-22T20:32:32Z

No worries! As it turns out, reading more of the documentation I think git won't fetch that ref by default, so CI is likely going to fail and we'll need to summon @benesch to do the manual thing again.

johanbrandhorst · 2017-12-22T21:16:47Z

Seems like CI worked after all?

tamird · 2017-12-22T21:17:23Z

Heh, what do you know, it worked. @benesch, should we just push @johanbrandhorst's rev to vendored/master and merge this?

tamird · 2017-12-22T21:18:38Z

pkg/util/protoutil/jsonpb_marshal.go

@@ -133,3 +133,8 @@ func (j *JSONPb) NewEncoder(w io.Writer) gwruntime.Encoder {
 		return errors.Errorf("unexpected type %T does not implement %s", v, typeProtoMessage)
 	})
 }
+
+// Delimiter implements gwruntime.Delimited.
+func (*JSONPb) Delimiter() []byte {


Oh, we can add the static assertions now, right?

Let's squash these commits while we're at it - no need to do it in two steps.

Ah, of course. Will do :).

Implement the Delimited interface for the gogo/protobuf JSONPb marshaller. Fixes cockroachdb#20925

johanbrandhorst · 2017-12-22T21:29:24Z

Adjustments made, thanks for your patience in what I expected to be a very simple change. Happy holidays :).

tamird · 2017-12-22T21:34:53Z

Thanks @johanbrandhorst! @benesch is going to finish this up tonight - you're all good. Happy holidays!

benesch · 2017-12-23T05:18:36Z

Thanks for all your hard work, @johanbrandhorst!

tamird reviewed Dec 21, 2017

View reviewed changes

johanbrandhorst force-pushed the patch-1 branch from 84ef846 to 7d2b9f8 Compare December 21, 2017 22:12

bdarnell reviewed Dec 21, 2017

View reviewed changes

johanbrandhorst force-pushed the patch-1 branch from 7d2b9f8 to c6c21c5 Compare December 21, 2017 23:13

johanbrandhorst mentioned this pull request Dec 22, 2017

Bump grpc-gateway version cockroachdb/vendored#13

Merged

johanbrandhorst force-pushed the patch-1 branch from c6c21c5 to 8ed9eaa Compare December 22, 2017 11:24

johanbrandhorst requested a review from a team December 22, 2017 11:24

johanbrandhorst force-pushed the patch-1 branch from 8ed9eaa to e61ea0b Compare December 22, 2017 16:49

johanbrandhorst commented Dec 22, 2017

View reviewed changes

johanbrandhorst force-pushed the patch-1 branch 2 times, most recently from 7430b65 to 295270c Compare December 22, 2017 18:41

tamird reviewed Dec 22, 2017

View reviewed changes

johanbrandhorst force-pushed the patch-1 branch from 295270c to 9f8c8dc Compare December 22, 2017 20:27

tamird reviewed Dec 22, 2017

View reviewed changes

Implement grpc-gateway Delimited interface

d1a8109

Implement the Delimited interface for the gogo/protobuf JSONPb marshaller. Fixes cockroachdb#20925

johanbrandhorst force-pushed the patch-1 branch from 9f8c8dc to d1a8109 Compare December 22, 2017 21:27

benesch merged commit c78c6df into cockroachdb:master Dec 23, 2017

		name = "github.com/grpc-ecosystem/grpc-gateway"
		branch = "master"

Implement grpc-gateway Delimited interface #20983

Implement grpc-gateway Delimited interface #20983

Conversation

johanbrandhorst commented Dec 21, 2017 • edited Loading

cockroach-teamcity commented Dec 21, 2017

tamird commented Dec 21, 2017

johanbrandhorst commented Dec 21, 2017

tamird commented Dec 21, 2017 • edited Loading

tamird Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Dec 21, 2017

johanbrandhorst commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamird commented Dec 21, 2017 • edited Loading

johanbrandhorst commented Dec 21, 2017

tamird commented Dec 21, 2017

johanbrandhorst commented Dec 21, 2017

tamird commented Dec 22, 2017

johanbrandhorst commented Dec 22, 2017

johanbrandhorst commented Dec 22, 2017

johanbrandhorst commented Dec 22, 2017

benesch commented Dec 22, 2017

tamird commented Dec 22, 2017 • edited Loading

benesch commented Dec 22, 2017 via email

johanbrandhorst commented Dec 22, 2017

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

johanbrandhorst commented Dec 22, 2017

tamird commented Dec 22, 2017

johanbrandhorst commented Dec 22, 2017

tamird commented Dec 22, 2017 • edited Loading

johanbrandhorst commented Dec 22, 2017 • edited Loading

tamird commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Dec 22, 2017

tamird commented Dec 22, 2017

johanbrandhorst commented Dec 22, 2017 • edited Loading

tamird commented Dec 22, 2017

johanbrandhorst commented Dec 22, 2017

tamird commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Dec 22, 2017

tamird commented Dec 22, 2017

benesch commented Dec 23, 2017

johanbrandhorst commented Dec 21, 2017 •

edited

Loading

tamird commented Dec 21, 2017 •

edited

Loading

tamird Dec 21, 2017 •

edited

Loading

tamird commented Dec 21, 2017 •

edited

Loading

tamird commented Dec 22, 2017 •

edited

Loading

tamird commented Dec 22, 2017 •

edited

Loading

johanbrandhorst commented Dec 22, 2017 •

edited

Loading

johanbrandhorst commented Dec 22, 2017 •

edited

Loading