-
Notifications
You must be signed in to change notification settings - Fork 222
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
Add version in store's meta information #285
Conversation
proto/pdpb.proto
Outdated
@@ -53,12 +53,15 @@ service PD { | |||
message RequestHeader { | |||
// cluster_id is the ID of the cluster which be sent to. | |||
uint64 cluster_id = 1; | |||
string version = 2; | |||
string require_min_version = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some comments here.
I prefer using number (major, minor, patch, unstable) for version, not string Btw, containing the version in every request is heavy? |
@siddontang Differs from your point of view, I think string is better. It's more readable, easier to construct a message, and more convenient to compare by using appropriate package (e.g. https://github.com/blang/semver) |
@disksing If we're serializing this onto every request I would be interested if there is a size difference between choosing string or a |
@Hoverbear I don't think the difference will be too significant. Notice that protobuf needs to use an extra varint to marshal each field. |
Actually, I compare them via:
The Result:
|
PTAL @disksing @siddontang |
LGTM. Please use a more appropriate PR title. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
For compatibility upgrade.