vtprotobuf icon indicating copy to clipboard operation
vtprotobuf copied to clipboard

Unable to get pooling methods to gen

Open joe-elliott opened this issue 3 years ago • 8 comments

I am currently attempting to put together a POC for using vtprotobuf in an application I work on. However the most promising feature (pooling) does not seem to exist in the generated code.

My command line:

protoc \
  -Ipkg/.patched-proto \
  --go_out=paths=source_relative:./pkg/tempopb/ \
  --go-grpc_out=paths=source_relative:./pkg/tempopb/ \
  --go-vtproto_out=paths=source_relative:./pkg/tempopb/ \
  --go-vtproto_opt=features=marshal+unmarshal+size+pool \
  pkg/.patched-proto/trace/v1/trace.proto

The output files seem to be generated correctly and there are no errors:

image

But I'm not seeing ResetVT, ReturnVTToPool, or *FromVTPool generated. I have tried with 0.2.0 as well as tip of main. I have also tried not specifying --go-vtproto_opt without luck.

I am seeing MarshalVT, MarshalToVT, SizeVT, UnmarshalVT, ... generated.

Thanks for your time!

joe-elliott avatar Mar 10 '22 18:03 joe-elliott

ok, I see now that I am supposed to use this option:

https://github.com/planetscale/vtprotobuf/blob/main/testproto/pool/pool.proto#L7

Struggling to reference the ext.proto file correctly, but messing around with it.

joe-elliott avatar Mar 10 '22 20:03 joe-elliott

Ok, I was able to get this to work by copying the ext.proto file into my project and then reference it from my proto. go mod vendor doesn't seem to want to bring this file in otherwise.

My file structure:

./pkg/tempopb/vtproto/ext.proto
./pkg/tempopb/tempo.proto

And then I am referencing it from tempo.proto like this:

import "pkg/tempopb/vtproto/ext.proto";

Is there a better way to do this that doesn't involve forking ext.proto into my repo?

joe-elliott avatar Mar 10 '22 20:03 joe-elliott

@joe-elliott You can use --go-vtproto_opt=pool=<package>.<struct> arguments to protoc to specify exactly which generated structs should have pooling enabled. This way you don't need to use any proto extensions.

npordash avatar Mar 10 '22 23:03 npordash

Yes, that's how we run the compilation in Vitess itself. Just use the --go_vtproto_opt CLI flag to enable specific options without having to modify the original .proto files. That will allow for fully backwards compatible ProtoBufs that can work with and without VTProtoBuf (in case you need to rollback or encounter bugs). Cheers!

vmg avatar Mar 11 '22 09:03 vmg

awesome, thx all!

joe-elliott avatar Mar 11 '22 13:03 joe-elliott

I've been unable to get these methods to compile, I've tried for type X in package y (no namespace/prefixing to do)

--go-vtproto_opt=pool=y.x --go-vtproto_opt=features=pool+marshal+unmarshal+size

but no pool helpers - it does something though, it causes the Server/Client interface types to double-generate in both the .vt.pb files and the regular .pb files. Any chance of a super minimal example @vmg to help, or if @joe-elliott you could share a working example?

I'm happy to return it back as a PR into the README.md if I can see it working.

steve-gray avatar Mar 23 '22 19:03 steve-gray

@steve-gray Vitess has been using that flag to generate our protos for a year now, and it's been working well for us. Here's a link to our Makefile:

https://github.com/vitessio/vitess/blob/main/Makefile#L243-L253

You can see that the objects passed to the flag must contain their full package namespace. Does this help fix your issues? How can we make the documentation easier to follow?

vmg avatar Mar 30 '22 14:03 vmg

Under each "available feature" in the README you could include how to use it in the protoc option. I came here after hitting the same issue.

jzelinskie avatar Sep 14 '22 20:09 jzelinskie