-
Notifications
You must be signed in to change notification settings - Fork 803
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
Demonstrate a way to get rid of the cadence-idl repo #5197
Draft
Groxx
wants to merge
10
commits into
cadence-workflow:master
Choose a base branch
from
Groxx:custom-proto-package
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaaagh dangit:
that's probably going to be fatal to this. |
well. it requires rewriting both proto and generated code, because yarpc is insufficiently configurable, but it's pretty simple and it does seem to work. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed
Cadence-server proto type X now no longer conflicts with cadence-client proto type X, because the server package name is implicitly rewritten to a different namespace.
This intentionally allows client and server protobufs to diverge, e.g. when developing a new server API that is not yet ready for client-side use, or when using different code-generator configuration.
Why?
This is a proof-of-concept for solving the issue found in cadence-workflow/cadence-idl#94 by doing the opposite resolution.
There are definitely some things to like about centralizing code-gen... quite a lot actually... but it sometimes causes a lot of unnecessary friction. It requires client and server to update their (client-)APIs in lock-step, even though new APIs may not yet be ready for use, and that might force upgrades that are not yet ready.
It also doesn't accurately represent how RPC works - on-the-wire binary compatibility has literally nothing to do with code compatibility, but this (and gogoproto's global registration in general) ties them together as if they are identical. It also implies that literally all code generation for proto file X is identical to every other - the very fact that their code-gen is configurable makes that unambiguously wrong.
So let's just stop doing that.
Tests
E2E tests exercise both thrift (unaffected) and proto, but I should mechanically assert that grpc client + server, and both client->server and server<->server are tested.
Potential risks
Three of them probably:
google.protobuf.Any
, which I do not believe is an issuegoogle.protobuf.Any
This works by essentially encoding
where the
type
value is selected by the creator of theAny
when encoding, and checked by the recipient when decoding... which this now changes, so the client-type will not be .Any-compatible with the server-type.There are simple ways to work around this, e.g. by looking up the type and manually decoding: https://github.com/gogo/protobuf/blob/f67b8970b736e53dbd7d0a27146c8f1ac52f74e5/types/any.go#L127
It's somewhat annoying that it has to be done, but entirely possible. And a reimplementation of ^ that method but with our customized comparison would be utterly trivial and would allow receiving
Any
essentially normally. Sending anAny
from server to client is similarly trivial but slightly manual.That said, personally I don't think we'll run into this.
Any
is almost never an accurate modeling of a domain: a field which contains literally any proto type, but not thrift or any other binary blob, and also not just protos we define and control, is... basically nonsense. Particularly for a multi-protocol system like Cadence.We can get the same capabilities without the limitations by using a custom
Any
that supports thrift/json/etc, and basically every other scenario has strictly more correct options (true request forwarding, aOneOf
, etc).grpc/yarpc issues
I believe E2E tests show this continues to work.
code-gen changes in the future
Inevitably annoying if this happens, but we pin our generator versions + yarpc's code-gen is very stable in practice. We can probably also convince them to allow modifying server names via config args, as in principle the proto files used have no technical relation whatsoever to how they are used - it's just commonly the same everywhere.
Follow-ups
Since this allows dropping cadence-idl use for server builds, where both types are used, we can also drop it in the client. This will need to be a v2 thing as it changes import paths, but it's not hard to re-introduce the necessary code-gen over there.
When fully complete though, this will turn into:
Which leaves things in a clear state for what depends on what, and at what version.