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

Feature Request: Replace SimulatedNull code with new protobuf optionals #15627

Open
mattlord opened this issue Apr 3, 2024 · 0 comments
Open

Comments

@mattlord
Copy link
Contributor

mattlord commented Apr 3, 2024

Feature Description

In the past the protobuf implementation of "optional" fields and testing for whether or not a field value was actually provided in a message relied on the one of feature which was not fully supported in the go protobuf implementation.

It was for this reason that the SimulatedNull support was added in Vitess and used in some of the new vtctldclient command implementations (all VReplication related): https://github.com/search?q=repo%3Avitessio%2Fvitess%20SimulatedNull&type=code
This was always clunky and ugly but in the past there was no less bad option.

In recent protobuf versions, however, the go implementation does now support the ability to detect if a field was specified/present in a message: https://github.com/protocolbuffers/protobuf/blob/v3.25.0/docs/field_presence.md

With recent versions (3.15+(?), but it was 3.2+ where this support was moved out of the experimental designation) when you mark a field as optional in the proto message definition, the given type becomes a pointer (e.g. string becomes *string). This allows for detection of a value being present as you can then leverage nil for pointers and differentiate from the type's ZeroValue.

We should move all of the SimulatedNull related code over to using this new optional protobuf behavior/feature. Open concerns are:

  1. Is adding the optional keyword to trigger this new behavior in recent protobuf versions backwards compatible?
  2. You cannot address consts, so we'd need to find a way around that as we often use them in the messages today (e.g. binlogdata.VReplicationWorkflowState)
  3. web/vtadmin/bin/generate-proto-types.sh fails with optional fields today
  4. optional does not seem to be supported with repeated

We definitely want to make this move as managing the SimulatedNull behavior is a pain and it's too easy to make a mistake (a new field is added in the message and existing callsites are not updated to pass the SimulatedNull value so you get unexpected results/behavior). It's just a matter of when we do this and how much work it will be.

Use Case(s)

Good code?

@mattlord mattlord self-assigned this Apr 3, 2024
@mattlord mattlord added this to Backlog in VReplication via automation Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

1 participant