-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
*: WatchResponse for multiple Events with WatchID #4122
Conversation
Please take a look. Thanks! |
int64 watchID = 3; | ||
} | ||
|
||
message WatchResponse { |
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.
we do not need this proto message. we should use the WatchResponse proto defined in rpc.proto in etcdserverpb pkg.
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.
move this back to watcher.go. We only need proto when we want to send it over wire. This type is just an internal message between two pkgs inside on process.
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.
Makes sense. I will just import etcdserverpb.WatchResponse
type.
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.
No. Do not import that type. Just create WatchResponse in watcher.go as what I have did in my original commit. Do not use proto type unless you are going to marshal the exact struct.
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.
Ok. Thanks,
@xiang90 @heyitsanthony Please take a look again. I removed all unnecessary changes |
Let squash all commits into one. |
LGTM after squashing. |
OK I will squash then into one. Thanks! Sincerely,
|
storage/storagepb: remove watchID from Event storage: add WatchResponse to watcher.go to wrap events, watchID storage: watchableStore to use WatchResponse storage: kv_test with WatchResponse etcdserver/api/v3rpc: watch to receive storage.WatchResponse type
@xiang90 Just squashed them all. Will merge after CI passes. Thanks, |
*: WatchResponse for multiple Events with WatchID
Reference: xiang90@ce07ae9