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

Replace gogo protobuf with google's protobuf v2 compiler #119

Merged
merged 61 commits into from
Nov 21, 2023

Conversation

tdeebswihart
Copy link
Contributor

@tdeebswihart tdeebswihart commented Oct 3, 2023

What changed?

gogo/protobuf has been replaced with Google's official go compiler.

⚠️ I have, for now, deleted our custom JSON coder so I can spread this change across our repos as quickly as possible for testing. I'd like to know ASAP whether we lose measurable performance due to this change. If we do, I will be introducing Vitess' protobuf compiler plugin vtproto

Why?

gogo/protobuf has been deprecated for some time and the community is moving on, building new tools (like vtproto) atop google's v2 compiler.

Breaking changes

  • *time.Time in proto structs will now be timestamppb.Timestamp
  • *time.Duration will now be durationpb.Duration
  • V2-generated structs embed locks, so you cannot dereference them willy-nilly. go vet will scream at you about this
  • Proto enums will, when formatted to JSON, now be in SCREAMING_SNAKE_CASE rather than PascalCase. We decided (in discussion with the SDK team) that now was as good a time as any to rip the bandage off

Note that history loading will not be impacted by the JSON changes: I rewrote history loading to dynamically fix incoming history JSON data (like all our other sdks); you can find this code in internal/temporalhistoryv1/load.go. That will be used by our Go sdk as well as our CLI when I get there.

How did you test it?

All tests for this repo pass as do all SDK tests (except for integration tests, which time out on our official repo on master...)

Potential risks

All errors that could arise from this change should be compile-time errors, so your build will be broken if I haven't yet addressed your code. I plan to port all relevant temporal repos as a part of this effort.

In addition the enum-rewriting code (the fix-enums makefile target) is rather heinous; if the official go plugin for protobuf ever changes how they name their enums (which they shouldn't) it will break.

Work to be done

  • Rewrite the custom JSON coding logic to use protojson Will be done later and moved to the server repo

This wasn't too bad, though the variable names disappeared from the
go-parsed type signature of our RPC specs so I had to generate variable
names manually /shrug
Google's protobuf compiler doesn't allow you to customize JSON
formatting and forking it like we did with gogo's is no simple
matter (it depends on a large number of internal packages...).

I'm deleting it for now to unblock myself while I wait to discuss this
with our HTTP API team tomorrow
Props to sed for existing
Go's protobuf-generated code embeds locks, so by copying the contents of
the object we violate the lock's guarantees. We'll return the new
payload and overwrite the old instead
The files changed name, whoops.
We'll want to use this in the CLI as well to allow users to update old histories
There's one option and its pretty much always supplied; this was an
over-eager change.
Pulled these over form the SDK and cleaned up the naming.
common.ThingPtr and common.ThingValue don't roll off the tongue as
nicely as thing.Proto and thing.Value.
Filter out non-go files and make sure to rewrite uses of these enums
Tests are now run by default (as they should be)
batch/v1/message.go-helpers.go Show resolved Hide resolved
cmd/proxygenerator/interceptor.go Show resolved Hide resolved
cmd/proxygenerator/service.go Outdated Show resolved Hide resolved
cmd/proxygenerator/service.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
internal/temporalhistoryv1/load.go Outdated Show resolved Hide resolved
internal/temporalhistoryv1/load.go Outdated Show resolved Hide resolved
internal/temporalhistoryv1/load.go Outdated Show resolved Hide resolved
types/duration/helpers.go Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would totally support the complete removal of mocks from this library, but not sure what others think

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think it makes sense to delete them for the following reasons:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone should file a ticket for it so we can put it somewhere in the backlog

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project doesn't have an issue tracker. What's the right place? Perhaps https://github.com/temporalio/sdk-go ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is already an issue for this temporalio/sdk-go#61.

We're back to proto for codegen too
My previous approach failed to fix enums in nested failures.
Rather than hardcode logic for handling failures specifically I took a
step back and approached it another way entirely: tracking paths to fix
for each proto message that is reachable from the History message.

With this approach we can handle any future type that can contain itself
without code changes
@ash2k
Copy link

ash2k commented Oct 10, 2023

"Kicking the tires" and gogo protobufs are causing a compilation failure in my project. I'm using bazel, so there are a few "gotchas", but... it'd be nice if everything just used the v2 vanilla API. Thank you!

@tdeebswihart
Copy link
Contributor Author

"Kicking the tires" and gogo protobufs are causing a compilation failure in my project. I'm using bazel, so there are a few "gotchas", but... it'd be nice if everything just used the v2 vanilla API. Thank you!

That's the plan. This effort is to use the vanilla v2 API across all temporal Go repos :)

@tdeebswihart
Copy link
Contributor Author

Note: since the HTTP API isn't GA yet I don't plan to include shorthand JSON support in the first pass of all this work.

I spoke with @yiminc yesterday about this and we can add that in after the initial release

@tdeebswihart
Copy link
Contributor Author

Note: since the HTTP API isn't GA yet I don't plan to include shorthand JSON support in the first pass of all this work.

I spoke with @yiminc yesterday about this and we can add that in after the initial release

And we chatted again today. The shorthand JSON support will almost certainly be in the initial release, but I'll reimplement it in the server repo as @cretz mentioned it's the only place we need it

@tdeebswihart tdeebswihart marked this pull request as ready for review October 27, 2023 22:50
@tdeebswihart tdeebswihart requested review from a team as code owners October 27, 2023 22:50
.gitmodules Outdated
@@ -1,3 +1,4 @@
[submodule "proto/api"]
path = proto/api
url = https://github.com/temporalio/api
url = https://github.com/tdeebswihart/temporal-api
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goes without saying I'm sure, but reminder to replace this when temporalio/api#317 merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

build/tools.go Outdated Show resolved Hide resolved
cmd/enumrewriter/main.go Outdated Show resolved Hide resolved
cmd/enumrewriter/main.go Show resolved Hide resolved
cmd/enumrewriter/main.go Show resolved Hide resolved
temporalproto/decode.go Show resolved Hide resolved
temporalproto/decode.go Outdated Show resolved Hide resolved
temporalproto/decode.go Outdated Show resolved Hide resolved
temporalproto/public_methods.go Outdated Show resolved Hide resolved
workflowservicemock/v1/service_grpc.pb.mock.go Outdated Show resolved Hide resolved
I've no idea why these two mocks keep swapping names. It's irritating
This is only used by the server so should stay there
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except for newly added dependency on strcase (https://github.com/temporalio/api-go/pull/119/files#r1376163691)

@tdeebswihart tdeebswihart merged commit 5a4d95c into temporalio:master Nov 21, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants