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

Please provide code backward compatibility / path for upgrade #1277

Closed
pattyshack opened this issue Jun 2, 2017 · 11 comments · Fixed by #1331
Closed

Please provide code backward compatibility / path for upgrade #1277

pattyshack opened this issue Jun 2, 2017 · 11 comments · Fixed by #1331
Assignees
Labels
Type: Feature New features or improvements in behavior

Comments

@pattyshack
Copy link

Please answer these questions before submitting your issue.

What version of gRPC are you using?

Upgrading from 3 to 4

What version of Go are you using (go version)?

1.6 / 1.7 / 1.8

What operating system (Linux, Windows, …) and version?

(Linux) This is a software engineering discipline issue.

What did you do?

If possible, provide a recipe for reproducing the error.

It's impossible to upgrade grpc without upgrading every single package in the repo. While this works for small projects, it's not practical for large projects.

For context, we have several hundred packages using protobuf/grpc. Many of these packages are thirdparty and are not controlled by us (i.e., we cannot dictate the pace of grpc upgrade)

What did you expect to see?

The ability to upgrade packages piecemeal. Ideally, grpc should support rolling upgrades (e.g., SupportPackageIsVersion3 & SupportPackageIsVersion4 at the same time, then 4 & 5, then 5 & 6, etc)

What did you see instead?

grpc.SupportPackageIsVersion* forces all packages to be upgraded at the same time. (I ended up hacking the generated code in order to support piecemeal upgrades)

@hsaliak
Copy link

hsaliak commented Jun 6, 2017

@dfawley could you please take a look and comment

@menghanl
Copy link
Contributor

Sorry about breaking backward compatibility on SupportPackageIsVersion.

SupportPackageIsVersion is just one of the public APIs of gRPC, though this one is a bit special because it involves interaction between gRPC package and proto codegen.

If your project uses some APIs in gRPC that got removed or modified in some release, you will need to deal with a similar problem as this. The difference is that, when changing other APIs, we try to not break backward compatibility. But for SupportPackageIsVersion, we have to remove the old one and there's no easy way to walk around.

So this problem is actually not specific to SupportPackageIsVersion*, or gRPC. It's a general question about package management for golang.
The official solution for this is vendor, and there are also a lit of options at: https://github.com/golang/go/wiki/PackageManagementTools.

@pattyshack
Copy link
Author

pattyshack commented Jun 13, 2017

like google, we use bazel/blaze to manage target building/testing. Unlike google, we do not compile go proto code on the fly via genfile (most of our BUILD files are auto-generated via code dependencies analysis tools; checking in the go proto code simplifies the toolchain).

vendor does not play nicely with bazel (we have mentioned this issue to the golang core team previously). how does google solve this general issue internally?

@pattyshack
Copy link
Author

btw, pinning via vendor would not solve the issue either since that still requires all thirdparty packages to upgrade to the latest version at once (this is pretty impractical since we rely on several hundred thirdparty packages ...)

@pattyshack
Copy link
Author

pattyshack commented Jun 13, 2017

I guess I didn't answer the real question. Most stable thirdparty packages does a pretty good job of ensuring backward compatibility. If I grab a new snapshot of a non-proto/grpc thirdparty package, things will generally compile.

grpc on the other hand intentionally breaks backward compatibility. By defining only SupportPackageIsVersion(N), anything that was generated with SupportPackageIsVersion(N-1) will break unless it's upgraded at the same time. (It's less of an issue for the proto pkg since the versions are more stable)

google3 component have to deal with this exact same issue (not sure if the project is still alive since Mike Borrows is now working on brains). When advancing component version, the api is backward compatibility until the older api usage is fully replaced with the new api.

@menghanl
Copy link
Contributor

menghanl commented Jun 13, 2017

As you mentioned, google recompiles proto files on the fly, so SupportPackageIsVersion is not a problem.
In google3, if we cannot ensure backward compatibility, we can make a big change to update all users to the latest API. It's time consuming, so most time keeping backward compatibility would be a good idea.

I understand that those may not be practical for external users. And I think the solution here is probably still vendoring.

I'm not sure what the issue about "vendor does not play nicely with bazel" was, but it seems the bazel team is make progress on supporting vendor:
https://github.com/bazelbuild/rules_go/blob/master/Vendoring.md#vendoring
bazelbuild/rules_go#488

@pattyshack
Copy link
Author

pattyshack commented Jun 13, 2017

Hmm, sounds like grpc usage within google is small enough that you can still rewrite everything in a single cl. Imagine grpc have replaced all of stubby3 (or pretend this project was bigtable or something), and your team needs to make api breaking changes. It would be impossible to rewrite everything at once, even with the help of rosie. All I'm saying is that your project would need to solve this issue anyways for google internal usage, so why not help us out. =P

We build everything at head for the same reason everything at google is built at head: it ensures that every project within to ecosystem has the latest/greatest fixes. It's significantly harder to ensure every service have all the latest fixes if each service has its own vendor set.

Specific to bazel, yes, you can compile vendor-ed packages, but that trashes your build cache and significantly increases compilation time.

@menghanl
Copy link
Contributor

About this specific issue on SupportPackageIsVersion3 & SupportPackageIsVersion4, the PR is: #941. The only thing affected is server reflection. So if you don't care about server reflection, you could ignore the version change (hard code version to 4 in old code generated with version 3), but all reflection requests will fail with "invalid file descriptor" error.
I would expect other things to work fine.

We try to keep backward compatibility when making API changes. It's a bit hard to do for SupportPackageIsVersion though. If we need to change it again (hopefully we won't, but...), we will try to change it in a backward compatible way, for example support rolling upgrades as you suggested.

Again, sorry for the breakage.

@pattyshack
Copy link
Author

Thanks for looking into this!

@pattyshack
Copy link
Author

pattyshack commented Jun 13, 2017

o, in case you need to roll a new version in the future, one way to provide backward compatibility is to add a Version field to ServiceDesc and populate that field as part of codegen (if the field is not populated, the lib can assume it's v4 or whatever). the grpc lib can check the version field and behave differently depending on the version value. just food for thought.

@menghanl
Copy link
Contributor

An update on this: it's not hard to add support for SupportPackageIsVersion3 back. So gRPC will support both SupportPackageIsVersion3 and SupportPackageIsVersion4.

We will get a PR out for this shortly.

@dfawley dfawley added the Type: Feature New features or improvements in behavior label Aug 24, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants