-
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
config: initial config protocol #498
Conversation
Signed-off-by: Neil Shen <overvenus@gmail.com>
proto/configpb.proto
Outdated
rpc Update(UpdateRequest) returns (UpdateResponse) {} | ||
} | ||
|
||
message Error { |
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.
It's better to change the Error
to Status
like the RocksDB
status.
Signed-off-by: Neil Shen <overvenus@gmail.com>
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
proto/configpb.proto
Outdated
enum StatusCode { | ||
UNKNOWN = 0; | ||
FAILED = 1; | ||
STALE_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.
should be 2
?
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.
Should we add variant NOT_CHANGE
which represents the configuration do not change?
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
|
||
message Version { | ||
uint64 local = 1; | ||
uint64 global = 2; |
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.
Need some comments? Hard to know what local
and global
mean.
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
@@ -60,8 +53,8 @@ message ConfigEntry { | |||
message CreateRequest { | |||
uint64 cluster_id = 1; | |||
Version version = 2; | |||
bytes component_id = 3; | |||
Component component = 4; | |||
string component = 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 could keep in consistent with TiDB system table TIDB_CLUTER_INFO, use TYPE and ADDRESS to represent a cluster component node, which maybe make the user have a more understanding.
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.
It's not only for tidb but also for other services, I prefer to keep it generic.
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
proto/configpb.proto
Outdated
} | ||
|
||
message Status { | ||
enum StatusCode { |
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.
Using the name StatusCode
will generate StatusStatusCode
in rust, maybe Code
is ok.
Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
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
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
Signed-off-by: Neil Shen <overvenus@gmail.com>
This PR adds config protocol.