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 Protobuf APIV2 #222

Closed
marwan-at-work opened this issue Mar 2, 2020 · 5 comments · Fixed by #304
Closed

Support Protobuf APIV2 #222

marwan-at-work opened this issue Mar 2, 2020 · 5 comments · Fixed by #304
Labels
pending Keeps issues from going stale

Comments

@marwan-at-work
Copy link
Contributor

The Go blog just posted that Protobuf's v2 work is officially released: https://blog.golang.org/a-new-go-api-for-protocol-buffers

While I haven't tried it out with Twirp yet, I just wanted to open an issue to track/answer some questions around this potentially breaking change.

The V1 of go's protobuf library has lived and still does in github.com/golang/protobuf while the new version now lives in google.golang.org/protobuf -- Therefore I will be referring to those import paths above in this issue to indicate which version I am referring to.

  1. Will Twirp adopting the new protobuf library mean a breaking change for everyone? The blog claims that golang/protobuf v1.4 uses google.golang.org/protobuf under the hood, so maybe that's the first step into migration?

  2. If golang/protobuf v1.4 actually does work with Twirp well, does that mean all Twirp clients must upgrade to v1.4 if the server is using v1.4?

  3. Should Twirp's new major version release (or the one after) incorporate generating the .twirp.go file with the new protobuf library?

  4. Are any of the new features mentioned in google.golang.org/protobuf useful for Twirp in a way that we couldn't do before? Such as printing defaults in json etc.

@marwan-at-work
Copy link
Contributor Author

cc @spenczar

@rhysh
Copy link
Contributor

rhysh commented May 6, 2020

This is a good set of questions. Thanks! Before getting into specifics, to make sure we're on the same page: Twirp generally has a bias towards pragmatism and keeping compatibility. Spencer set the tone in the announcement post, contrasting it on those two points with gRPC's requirement for http/2 and frequent refactors. Those remain important for Twirp's users (certainly the ones within Twitch).

That sets guidelines for what changes won't be part of Twirp—but what changes will? The benefit of the change needs to outweigh the bias for stability. Maybe v2 / "v1.20" has significantly faster (de-)serialization, particularly for binary mode. Maybe v1 gets deprecated and everyone needs to move. Maybe the community moves to v2, and the shims required for v1 compatibility make v1 performance much worse. Those could be enough impetus to move to v2.

  1. Yes, application owners specifying v1.4.0 instead of v1.3.x (or older) sounds like a safe first step. This is about what's in the go.mod file of an application, no interaction required from Twirp upstream.

  2. The protobuf wire protocol hasn't changed: client and server executables will still be able to communicate. Next, since client and server maintainers get to pick their own version of the github.com/golang/protobuf/proto package to use, they don't need to coordinate a move to v1.4.0. Beyond that, stability in the Twirp wire protocol and the protobuf binary serialization format mean that client and server maintainers don't even need to use the same generated code—they only need to share the same .proto source file. Again, sounds like Twirp upstream already has the support necessary here.

  3. Maybe, if there's sufficient benefit to a move and a good compatibility story. (Will this trigger a Twirp-ecosystem-wide migration to the new package?) What changes are required in the generated .twirp.go code? (Are they simple enough that eager early adopters can write short and safe sed scripts to make the changes?) What is the cost / performance overhead of not moving, and instead leaning on the compatibility outlined in the protobuf blog post?

  4. I've written some protobuf+reflection code for (closed-source) Twirp-adjacent tools, and that was tedious with the github.com/golang/protobuf/proto API. I expect it'll be easier to write correct code in that genre with the new google.golang.org/protobuf interfaces, though I haven't attempted to migrate yet. I don't immediately see how the core Twirp project would take advantage of the new APIs.

@cyx
Copy link

cyx commented Aug 25, 2020

Greetings,

We're working around this now by just shimming in a jsonpb compatible interface since we're using field mask and that doesn't work with jsonpb.

Will we get some buy in moving forward with this issue?

@github-actions
Copy link

github-actions bot commented Dec 3, 2020

This issue is stale because it has been open 60 days with no activity. Remove stale label / comment or this will be closed in 5 days

@github-actions
Copy link

github-actions bot commented Feb 2, 2021

This issue is stale because it has been open 60 days with no activity. Remove stale label / comment or this will be closed in 5 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending Keeps issues from going stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants