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 #8662

Closed
wants to merge 1 commit into from

Conversation

connor4312
Copy link
Contributor

@connor4312 connor4312 commented Oct 7, 2017

This allows for watchers to be created concurrently without needing complex and latency-adding queuing on the client.

I purposely didn't update clientv3 yet, in case consumers import an updated clientv3 but don't run the latest etcd release. This would cause subtly breaking behavior for them. Tested this end to end locally and it seems to work fine.

Resolves #7036

@connor4312
Copy link
Contributor Author

connor4312 commented Oct 7, 2017

The build failure seems unrelated to the changes in this PR, as I'm seeing it on previous PRs like this one which just updates the readme

mvcc/watcher.go Outdated
@@ -23,7 +23,8 @@ import (
)

var (
ErrWatcherNotExist = errors.New("mvcc: watcher does not exist")
ErrWatcherNotExist = errors.New("mvcc: watcher does not exist")
ErrWatcherDuplicateID = errors.New("mvcc: duplicate ID on the WatchStream")
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate watch ID provided on the WatchStream

//
// The whole event history can be watched unless compacted.
// If `startRev` <=0, watch observes events after currentRev.
//
// The returned `id` is the ID of this watcher. It appears as WatchID
// in events that are sent to the created watcher through stream channel.
//
Watch(key, end []byte, startRev int64, fcs ...FilterFunc) WatchID
Watch(id WatchID, key, end []byte, startRev int64, fcs ...FilterFunc) (WatchID, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to define the nonWatchID as a const.

mvcc/watcher.go Outdated
@@ -33,15 +34,17 @@ type FilterFunc func(e mvccpb.Event) bool

type WatchStream interface {
// Watch creates a watcher. The watcher watches the events happening or
// happened on the given key or range [key, end) from the given startRev.
// happened on the given key or range [key, end) from the given startRev. If
// the ID is provided and non-zero, it will be used, otherwise a new ID
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that -1 is the value, not non-zero.

Copy link
Contributor Author

@connor4312 connor4312 Oct 22, 2017

Choose a reason for hiding this comment

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

So, it actually is zero, because it doesn't look like the protobuf is set to have optional types (so, if the watcher ID is not present in the WatchRequest, it's zero). There is some kind of weird casing around 0 and -1 right now, since I didn't change the -1 return the watch creation function had previously.

Instead, I want to normalize around 0 and get rid of the -1 return in favor of a sentinel error.

// - Try to duplicate it, get an error
// - Make sure the auto-assignment skips over things we manually assigned

id, err := w.Watch(1, []byte("foo"), nil, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you convert this to a table driven test?

@xiang90
Copy link
Contributor

xiang90 commented Oct 10, 2017

lgtm in general.

@connor4312
Copy link
Contributor Author

Thanks for the review! I'll get it adjusted tonight

@xiang90
Copy link
Contributor

xiang90 commented Oct 22, 2017

@connor4312 kindly ping.

@connor4312
Copy link
Contributor Author

Hi, I've updated my PR. A few changes:

  • Remove the magic -1. Made empty/"bad" ranges return a sentinel error instead of signalling with -1.
  • The zero ID is the only magic ID, for which we'll assign an ID automatically (no way to tell from the protobuf the difference between a 0 and a not-present field)
  • Moved the bulk of the logic in recvLoop out to its own recvWatchRequest method which simplifies the control flow a good bit
  • Moved the mentioned test to a table driven test

mvcc/watcher.go Outdated
@@ -22,8 +22,14 @@ import (
"github.com/coreos/etcd/mvcc/mvccpb"
)

// DefaultWatchID is the watcher ID passed in WatchStream.Watch when no
// user-provided ID is available. If pass, an ID will automatically be assigned.
const DefaultWatchID WatchID = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

AutoWatchID? it is more descriptive. when auto watch id is set, etcd automatically assign watch id.

@xiang90
Copy link
Contributor

xiang90 commented Oct 24, 2017

@connor4312 looks good to me in general. can you rebase with the current master so test can pass? thanks.

This allows for watchers to be created concurrently without needing potentially complex and latency-adding queuing on the client
@connor4312
Copy link
Contributor Author

Done and done :)

@xiang90
Copy link
Contributor

xiang90 commented Nov 13, 2017

@connor4312 Can you rebase with the current master to let CI pass? Thanks!

@gyuho
Copy link
Contributor

gyuho commented Dec 14, 2017

@connor4312 Sorry for delay! Our CIs have been broken for so long, and finally it's fixed now.
I cherry-picked this patch to resolve conflicts and also to include this in our upcoming 3.3 release.

#9015

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