Conversation
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
# Conflicts: # pkg/kvrpcpb/kvrpcpb.pb.go # pkg/pdpb/pdpb.pb.go Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
# Conflicts: # pkg/keyspacepb/keyspacepb.pb.go # proto/keyspacepb.proto # scripts/proto.lock
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| // The keyspace that the request is sent to. | ||
| // NOTE: This field is only meaningful while the api_version is V2. | ||
| uint32 keyspace_id = 32; | ||
| oneof keyspace { |
There was a problem hiding this comment.
Changing this existing scalar field into a oneof is source-incompatible for generated Go users even though the wire tag is preserved. Context.KeyspaceId disappears, and downstream PD/TiDB/client-go code constructs or reads this field directly, so upgrading kvproto would break V1/V2 callers that do not use V3. The legacy scalar should stay as a normal field and keyspace_identity should be added alongside it, as was done for AutoIDRequest and KeyspaceMeta.
There was a problem hiding this comment.
Thanks, fair point. I originally kept keyspace_id as a normal field and added keyspace_identity alongside it to minimize downstream Go source changes. Later I changed it to oneof based on review feedback, since semantically it is clearer that only one keyspace identifier should be set.
I think both directions are reasonable: parallel fields are more source-compatible, while oneof is clearer semantically. @sunxiaoguang what do you think? If we prefer minimizing downstream changes for kvrpcpb.Context, I can change this back to the parallel-field style.
Summary
Validation