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

*: support get-old-kv in watch #5850

Merged
merged 1 commit into from
Jul 5, 2016
Merged

*: support get-old-kv in watch #5850

merged 1 commit into from
Jul 5, 2016

Conversation

xiang90
Copy link
Contributor

@xiang90 xiang90 commented Jul 2, 2016

/cc @heyitsanthony

I will rename the preserve-kv in delete to get-old-kv too. also i will probably add the same option for put.

@xiang90
Copy link
Contributor Author

xiang90 commented Jul 2, 2016

@ajityagaty You might want to review this.

@ajityagaty
Copy link
Contributor

@xiang90 LGTM
This addresses #4620 right?

@chancez
Copy link
Contributor

chancez commented Jul 2, 2016

Isn't old is a bit vague? Would prev or previous be more descriptive perhaps?

@@ -43,4 +43,7 @@ message Event {
// A DELETE/EXPIRE event contains the deleted key with
// its modification revision set to the revision of deletion.
KeyValue kv = 2;

// oldkv holds the KeyValue before the event happens when requested.
KeyValue oldKV = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

prev_kv?

@xiang90
Copy link
Contributor Author

xiang90 commented Jul 5, 2016

@chancez @heyitsanthony OK. I think we can rename old_kv to prev_kv. For delete, the original idea was named it as deleted_kv to be more specific and clear. But that does not make sense any more since we decided to provide the prev_kv for watch/delete/put.

@xiang90
Copy link
Contributor Author

xiang90 commented Jul 5, 2016

/cc @hongchaodeng

@@ -253,6 +268,14 @@ func (sws *serverWatchStream) sendLoop() {
events := make([]*mvccpb.Event, len(evs))
for i := range evs {
events[i] = &evs[i]

if sws.getOldKV[wresp.WatchID] {
Copy link
Contributor

Choose a reason for hiding this comment

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

do map lookup before loop?

@heyitsanthony
Copy link
Contributor

approach looks fine, but not a fan of the naming

@xiang90 xiang90 force-pushed the get_o_kv branch 2 times, most recently from 4b453d5 to f7f13c7 Compare July 5, 2016 19:13
@xiang90
Copy link
Contributor Author

xiang90 commented Jul 5, 2016

@heyitsanthony @chancez All fixed. PTAL.


// If prevKV is set, created watcher gets the previous KV before the event happens.
// If the previous KV is already compacted, nothing will be returned.
bool prevKV = 6;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be prev_kv, not prevKV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

@heyitsanthony
Copy link
Contributor

lgtm after protobuf naming style fixups

@xiang90 xiang90 merged commit 929d6ab into etcd-io:master Jul 5, 2016
@xiang90 xiang90 deleted the get_o_kv branch July 5, 2016 23:37
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.

4 participants