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

mvcc: allow clients to assign watcher IDs #9015

Closed
wants to merge 7 commits into from
Closed

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Dec 14, 2017

Cherry-pick #8662.
Fix #7036.

/cc @connor4312

@xiang90
Copy link
Contributor

xiang90 commented Dec 14, 2017

let us wait for the 3.3 rc release. this should go into 3.4 branch not 3.3 release branch.

@gyuho
Copy link
Contributor Author

gyuho commented Dec 14, 2017

@xiang90 Sounds good. Will merge after 3.3 release branch cut.

@xiang90
Copy link
Contributor

xiang90 commented Dec 21, 2017

@gyuho let us get this merged into master?

@gyuho
Copy link
Contributor Author

gyuho commented Dec 21, 2017

@xiang90 Yeah let me rebase first and see if test passes.
Previously, some watch tests were failing.

UPDATE: investigating https://semaphoreci.com/coreos/etcd/branches/pull-request-9015/builds/3

--- FAIL: TestCtlV3AuthAndWatch (15.19s)
	testutil.go:55: goroutine 5054 [running]:
		github.com/coreos/etcd/pkg/testutil.FatalStack(0xc420012690, 0xc420014e00, 0x18)
			/go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/pkg/testutil/testutil.go:54 +0x74
		github.com/coreos/etcd/e2e.testCtl(0xc420012690, 0xd2c7d0, 0x0, 0x0, 0x0)
			/go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/e2e/ctl_v3_test.go:179 +0x4ac
		github.com/coreos/etcd/e2e.TestCtlV3AuthAndWatch(0xc420012690)
			/go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/e2e/ctl_v3_auth_test.go:42 +0x52
		testing.tRunner(0xc420012690, 0xd2c268)
			/usr/local/go/src/testing/testing.go:746 +0xd0
		created by testing.(*T).Run

This case fails

		{ // watch 1 key, should not be successful
			[]kv{},
			[]string{"key5", "--rev", "1"},
			[]kvExec{},
			false,
		},

To reproduce

ETCDCTL_API=3 ./bin/etcdctl user add root --interactive=false
root

ETCDCTL_API=3 ./bin/etcdctl user grant-role root root
ETCDCTL_API=3 ./bin/etcdctl auth enable

ETCDCTL_API=3 ./bin/etcdctl --user=root:root user add abc --interactive=false
def

ETCDCTL_API=3 ./bin/etcdctl --user=root:root role add test-role
ETCDCTL_API=3 ./bin/etcdctl --user=root:root user grant-role abc test-role
ETCDCTL_API=3 ./bin/etcdctl --user=root:root role grant-permission test-role readwrite foo

ETCDCTL_API=3 ./bin/etcdctl --user=root:root role grant-permission test-role readwrite key key4

ETCDCTL_API=3 ./bin/etcdctl --user=abc:def watch key5 --rev 1

<<COMMENT
expect
Error: watch is canceled by the server
COMMENT

@gyuho gyuho force-pushed the watch-id branch 2 times, most recently from 6a08c6e to fcd4c81 Compare December 22, 2017 05:40
@gyuho gyuho self-assigned this Dec 22, 2017
gyuho and others added 4 commits December 22, 2017 10:12
Signed-off-by: Gyuho Lee <[email protected]>
This allows for watchers to be created concurrently
without needing potentially complex and latency-adding
queuing on the client.

Signed-off-by: Gyuho Lee <[email protected]>
@@ -367,7 +367,7 @@ func (w *watcher) closeStream(wgs *watchGrpcStream) {
}

func (w *watchGrpcStream) addSubstream(resp *pb.WatchResponse, ws *watcherStream) {
if resp.WatchId == -1 {
if resp.Canceled && resp.CancelReason != "" {
Copy link
Contributor Author

@gyuho gyuho Dec 22, 2017

Choose a reason for hiding this comment

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

@xiang90 This PR breaks backward compatibility if we want to returning the original ID from watch create request.

<= v3.3: canceled watch request response always resp.WatchId == -1
>= v3.4: canceled watch request response may have watch ID.

So, the way to check cancellation changes. We could keep returning -1 as watch ID on cancellation, but clients won't be able to know which one is canceled...

@gyuho
Copy link
Contributor Author

gyuho commented Dec 22, 2017

Replaced by #9065.

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.

3 participants