planetscale / vtprotobuf

A Protocol Buffers compiler that generates optimized marshaling & unmarshaling Go code for ProtoBuf APIv2
BSD 3-Clause "New" or "Revised" License
876 stars 75 forks source link

Unable to get pooling methods to gen #33

Closed joe-elliott closed 1 year ago

joe-elliott commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

npordash commented 2 years ago

@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.

vmg commented 2 years ago

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!

joe-elliott commented 2 years ago

awesome, thx all!

steve-gray commented 2 years ago

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.

vmg commented 2 years ago

@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?

jzelinskie commented 2 years ago

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.