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

*: WatchResponse for multiple Events with WatchID #4122

Merged
merged 1 commit into from
Jan 4, 2016

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jan 3, 2016

Reference: xiang90@ce07ae9

  1. Return WatchResponse instead of []Events in stream chan.
  2. Attach WatchID into WatchResponse
  3. Remove WatchID from Event

@gyuho gyuho changed the title Watchid events [WIP] storage, storagepb: WatchResponse for multiple events with one WatchID Jan 3, 2016
@gyuho
Copy link
Contributor Author

gyuho commented Jan 3, 2016

/cc @xiang90 @heyitsanthony

Please take a look. Thanks!

@gyuho gyuho changed the title [WIP] storage, storagepb: WatchResponse for multiple events with one WatchID storage, storagepb: WatchResponse for multiple events with one WatchID Jan 3, 2016
int64 watchID = 3;
}

message WatchResponse {
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks,

@gyuho
Copy link
Contributor Author

gyuho commented Jan 3, 2016

@xiang90 @heyitsanthony Please take a look again. I removed all unnecessary changes
to keep this PR minimum.

@xiang90
Copy link
Contributor

xiang90 commented Jan 3, 2016

Let squash all commits into one.

@xiang90
Copy link
Contributor

xiang90 commented Jan 3, 2016

LGTM after squashing.

@gyuho
Copy link
Contributor Author

gyuho commented Jan 4, 2016

OK I will squash then into one. Thanks!

Sincerely,
Gyu-Ho Lee
On Jan 3, 2016 15:10, "Xiang Li" notifications@github.com wrote:

LGTM after squashing.


Reply to this email directly or view it on GitHub
#4122 (comment).

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
@gyuho gyuho changed the title storage, storagepb: WatchResponse for multiple events with one WatchID *: WatchResponse for multiple Events with WatchID Jan 4, 2016
@gyuho
Copy link
Contributor Author

gyuho commented Jan 4, 2016

@xiang90 Just squashed them all. Will merge after CI passes. Thanks,

gyuho added a commit that referenced this pull request Jan 4, 2016
*: WatchResponse for multiple Events with WatchID
@gyuho gyuho merged commit bf9e2a5 into etcd-io:master Jan 4, 2016
@yichengq yichengq mentioned this pull request Jan 4, 2016
1 task
@gyuho gyuho deleted the watchid_events branch January 31, 2016 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants