Expose vtprotobuf generation about go-control-plane HOT 7 CLOSED

Did a few discussions with folks about various risks and tradeoffs

The biggest concerns are around a gogo 2.0 that can't be ripped out easily.
This library is fundamentally different, and intentionally avoids this issue.
Calling looks like this: https://github.com/vitessio/vitess/blob/40f314c1d052db3fa5a52e5fb2ddd2bddaefde44/go/vt/servenv/grpc_codec.go#L40-L49, so there is no code changes required if its removed later. Theoretically you could break yourself, but you would have to use the library in a poor way to do so.
The original proto Go struct is unmodified.

From the internal protobuf side, I am not sure how much of the details I can discuss publicly, but there was not concerns in adoption of vtprotobuf.

from go-control-plane.

Calling looks like this: vitessio/vitess@40f314c/go/vt/servenv/grpc_codec.go#L40-L49, so there is no code changes

That's what calling code should look like. And what happens when an important project writes the equivalent of:

func (vtprotoCodec) Marshal(v any) ([]byte, error) {
	return v.(vtprotoMessage).MarshalVT() // panics if the proto doesn't support vtprotoMessage
}

Removing vtproto would be a breaking change.

from go-control-plane.

Calling looks like this: vitessio/vitess@40f314c/go/vt/servenv/grpc_codec.go#L40-L49, so there is no code changes

That's what calling code should look like. And what happens when an important project writes the equivalent of:

func (vtprotoCodec) Marshal(v any) ([]byte, error) {
	return v.(vtprotoMessage).MarshalVT() // panics if the proto doesn't support vtprotoMessage
}

Removing vtproto would be a breaking change.

You can go down this rabbit hole with any API really. Someone can always use reflect or unsafe to pull out data from private fields, or other crazy shenanigans.

If someone goes out of there way to shoot themselves in the foot, I don't see any issue with breaking them -- it would be far less of a breaking change than go-control-plane has made numerous times over the years as well (not reason to do it again, but still).

Also see build tag discussion in envoyproxy/envoy#31172 (comment)

from go-control-plane.

These aren't private fields, and my example does not use reflection or unsafe. These are exported methods, and removing them is going to be seen as less "okay".

The issue with "just break people who do bad things" is: when you work for a big company (e.g. Google) and someone imports the project doing bad things into your codebase, you might not be able to upgrade your thing without fixing the people that did the bad things first.

If this can go in with a build flag that is opt-in instead of opt-out, that would probably remove all of my concerns.

from go-control-plane.

If this can go in with a build flag that is opt-in instead of opt-out, that would probably remove all of my concerns.

This is my plan, need to merge planetscale/vtprotobuf#122 first though

from go-control-plane.

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

from go-control-plane.

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

from go-control-plane.