Skip to content
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

Support json names in field mask generation #1050

Merged
merged 2 commits into from
Sep 30, 2019
Merged

Support json names in field mask generation #1050

merged 2 commits into from
Sep 30, 2019

Conversation

william-plano-oxb
Copy link
Contributor

Added tests for using jsonpb with OrigName: false
Added benchmarks for FieldMaskFromRequestBody called with a message descriptor

Added tests for using jsonpb with OrigName: false
Added benchmarks for FieldMaskFromRequestBody called with a message descriptor
@william-plano-oxb
Copy link
Contributor Author

Thankfully this doesn't look like it's significantly slower than before:

BenchmarkABEFieldMaskFromRequestBodyWithDescriptor-12            	    3000	    373950 ns/op
BenchmarkNonStandardFieldMaskFromRequestBodyWithDescriptor-12    	  100000	     20738 ns/op

vs

BenchmarkABEFieldMaskFromRequestBody-12            	    5000	    337770 ns/op
BenchmarkNonStandardFieldMaskFromRequestBody-12    	  100000	     18189 ns/op

@johanbrandhorst
Copy link
Collaborator

I don't understand the bazel error well enough to say if it's real: https://circleci.com/gh/grpc-ecosystem/grpc-gateway/4473?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link @drigz @achew22 please bazel police come to my aid.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This includes a change to the runtime package, so we will likely have to release a new minor version immediately after merge to stem the flow of people raising issues that are rooted in installing the gateway the wrong way.

@william-plano-oxb
Copy link
Contributor Author

This includes a change to the runtime package, so we will likely have to release a new minor version immediately after merge to stem the flow of people raising issues that are rooted in installing the gateway the wrong way.

I'm not sure that I follow, but in any case a new release sounds good to me 😄

@johanbrandhorst
Copy link
Collaborator

I'm not sure that I follow, but in any case a new release sounds good to me smile

To give you an idea: #963, #1023

@william-plano-oxb
Copy link
Contributor Author

To give you an idea: #963, #1023

Aah okay, I understand
quickly checks his Dockerfile
I think I'm good 😁

@william-plano-oxb
Copy link
Contributor Author

I don't understand the bazel error well enough to say if it's real: https://circleci.com/gh/grpc-ecosystem/grpc-gateway/4473?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link @drigz @achew22 please bazel police come to my aid.

Are you able to poke it to re-run the workflow?

@johanbrandhorst
Copy link
Collaborator

Are you able to poke it to re-run the workflow?

Yes

@johanbrandhorst
Copy link
Collaborator

Are you able to poke it to re-run the workflow?

Yes

:(

@johanbrandhorst
Copy link
Collaborator

fatal error: error in backend: IO failure on output stream: Cannot allocate memory

This is surely a random error. I'll poke it again.

@william-plano-oxb
Copy link
Contributor Author

Are you able to poke it to re-run the workflow?

Yes

:(

Hrm, that seems worse ☹️

@william-plano-oxb
Copy link
Contributor Author

fatal error: error in backend: IO failure on output stream: Cannot allocate memory

This is surely a random error. I'll poke it again.

I do have some pretty long names in the tests here, they run fine locally, but maybe that's the issue?

@johanbrandhorst
Copy link
Collaborator

fatal error: error in backend: IO failure on output stream: Cannot allocate memory

This is surely a random error. I'll poke it again.

I do have some pretty long names in the tests here, they run fine locally, but maybe that's the issue?

Nah we get the occasional Bazel error, we're working on it: #968.

@william-plano-oxb
Copy link
Contributor Author

Okay, I ran bazel test //... locally and got some errors along the lines of:

compilepkg: missing strict dependencies:
	/private/var/tmp/_bazel_william.plano/907f576adbee9625b7b379068b79de6e/sandbox/darwin-sandbox/487/execroot/grpc_ecosystem_grpc_gateway/bazel-out/darwin-fastbuild/bin/examples/proto/examplepb/darwin_amd64_stripped/examplepb_go_proto%/github.com/grpc-ecosystem/grpc-gateway/examples/proto/examplepb/a_bit_of_everything.pb.gw.go: import of "github.com/golang/protobuf/descriptor"

This seems likely to be legitimate since I've added descriptor as a new dependency to the generated gw files. I'll try and work out what I need to put where to get it to work.

@drigz
Copy link
Contributor

drigz commented Sep 27, 2019

This seems likely to be legitimate since I've added descriptor as a new dependency to the generated gw files. I'll try and work out what I need to put where to get it to work.

Looks like you need to manually add it as a dep for the :examplepb_go_proto target. I don't know enough about Gazelle to know if this should have been done automatically, but you could try filing an issue.

patch -p1 << EOF
diff --git a/examples/proto/examplepb/BUILD.bazel b/examples/proto/examplepb/BUILD.bazel
index c652f7b..9e4011c 100644
--- a/examples/proto/examplepb/BUILD.bazel
+++ b/examples/proto/examplepb/BUILD.bazel
@@ -51,6 +51,7 @@ go_proto_library(
         "//examples/proto/sub:go_default_library",
         "//examples/proto/sub2:go_default_library",
         "//protoc-gen-swagger/options:go_default_library",
+        "@com_github_golang_protobuf//descriptor:go_default_library_gen",
         "@go_googleapis//google/api:annotations_go_proto",
     ],
 )
EOF

@william-plano-oxb
Copy link
Contributor Author

Thanks @drigz, that did the trick. I had to add a # keep comment to convince bazel not to throw it away, luckily there was a similar comment a few lines above to set precedent. I don't think I understand enough about what's going on here to form a coherent issue report.

@william-plano-oxb
Copy link
Contributor Author

Aargh, "it worked on my machine":tm:
I think we're maybe back to the random error again:

fatal error: error in backend: IO failure on output stream: Cannot allocate memory

@johanbrandhorst
Copy link
Collaborator

🔨

@johanbrandhorst johanbrandhorst merged commit 8eb1d0f into grpc-ecosystem:master Sep 30, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* Support json names in field mask generation
Added tests for using jsonpb with OrigName: false
Added benchmarks for FieldMaskFromRequestBody called with a message descriptor

* Add descriptor dependency to go_proto_library
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants