cockroachdb / cockroachdb/cockroach

*: migrate to google.golang.org/protobuf, decide on the fate of gogoproto

Open
#136,195 0 comments 1 reaction 0 assignees View on GitHub
A-build-system A-dependencies A-server-architecture branch-master C-investigation
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

`google.golang.org/protobuf` is (roughly speaking) the `v2` of `github.com/golang/protobuf`. The former is built around "protobuf reflection" and uses the following `proto.Message` interface:

```go
type Message interface {
ProtoReflect() protoreflect.Message
}
```

The (new) README[^1] is well written and explains why they changed the interface. The gist is that the old interface was mostly a marker interface and didn't really specify what was "expected" of types that are protobuf messages, and so wasn't really making sure that messages created by different libraries would interop cleanly. It was brittle, and they fixed it by changing to the above interface, which describes the _behavior_ of `Message`: `ProtoReflect()` lets you interrogate and manipulate the underlying message entirely through interfaces, without ever worrying about the backing Go types.

It looks quite slick and well thought out, however for us it poses the problem of dealing with `gogoproto`[^gogo]. Gogo is no longer maintained, and we use it instead of "vanilla" protobuf because it's relatively fast (for a protobuf solution) and has some nifty features, such as avoiding pointers (and the associated heap pressure) in many circumstances by allowing you to use "idiomatic" Go types.

The world, turning as it does, has seen various projects adopt `google.golang.org/protobuf`. So frequently, when we try to upgrade some dependency, we rediscover some version of the fact that you really need to tiptoe around our protobuf dependencies. As a recent example of this, here's a more-than-100-message-deep thread[^grpcthread] on trying to upgrade `gRPC`.

Whenever we upgrade any dependency to use v2 protos, it will expect v2 protos. However, `gogoproto` stopped being maintained before v2 proto comes out, so it always generates v1 protos. You'll thus see errors like this[^grpcthread]:

```
bazel-out/darwin_arm64-fastbuild/bin/pkg/ts/tspb/tspb_go_proto_/github.com/cockroachdb/cockroach/pkg/ts/tspb/timeseries.pb.gw.go:43:9: cannot use msg (variable of type *TimeSeriesQueryResponse) as protoreflect.ProtoMessage value in return statement: *TimeSeriesQueryResponse does not implement protoreflect.ProtoMessage (missing method ProtoReflect)
```

Without strong ownership of this part of the codebase, and shared infrastructure being hard to own by any individual team anyway, this problem has been allowed to fester for a few years. We should decide whether we stay on gogoproto or migrate off, and generally take steps that put us in a place in which upgrading key dependencies such as grpc is no longer a major effort.

[^grpcthread]: https://cockroachlabs.slack.com/archives/CJ0H8Q97C/p1732314011559849?thread_ts=1731420942.261169&cid=CJ0H8Q97C

[^gogo]: https://github.com/cockroachdb/gogoproto

[^1]: https://github.com/protocolbuffers/protobuf-go#historical-legacy

Some closing thoughts: there are a number of worlds we may choose to live in. These are not in any preference order.

- we could commit to gogoproto and do the work to make it play ball with protov2.
- we could migrate off of gogoproto on "critical paths" (BatchRequest etc), for example to flatbuffers or gob, and then just use vanilla protov2 everywhere else (since the loss of performance will be "ok")
- we could migrate completely onto protov2, but would likely have to maintain a fork to teach it some of gogoproto's tricks to avoid pointers.

I sense that there is also a partial way forward that allows us to adopt protov2 in dependencies without breaking the build. There is a reflection-based way to "dress up" v1 messages as v2:

[/protoadapt/convert.go#L27-L31](https://github.com/protocolbuffers/protobuf-go/blob/30f628eeb303f2c29be7a381bf78aa3e3aabd317/protoadapt/convert.go#L27-L31)
```go
// MessageV2Of converts a v1 message to a v2 message.
// It returns nil if m is nil.
func MessageV2Of(m MessageV1) MessageV2 {
return protoimpl.X.ProtoMessageV2Of(m)
}
```

This is likely going to be non-performant, but we _should_ be able to use this in places like `grpc-gateway@v2` which has been one of the roadblocks to upgrading in the past. This is likely worth pulling on, as it could allow us to "unblock" the dependency mess and decouple it from our ultimate decision about the fate of `gogoproto`.

Jira issue: CRDB-44913

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.