mvcc: allow clients to assign watcher IDs #8662
Conversation
|
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") | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
it seems that -1 is the value, not non-zero.
There was a problem hiding this comment.
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.
mvcc/watcher_test.go
Outdated
| // - 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) |
There was a problem hiding this comment.
can you convert this to a table driven test?
|
lgtm in general. |
|
Thanks for the review! I'll get it adjusted tonight |
|
@connor4312 kindly ping. |
|
Hi, I've updated my PR. A few changes:
|
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 | |||
There was a problem hiding this comment.
AutoWatchID? it is more descriptive. when auto watch id is set, etcd automatically assign watch id.
|
@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
|
Done and done :) |
|
@connor4312 Can you rebase with the current master to let CI pass? Thanks! |
|
@connor4312 Sorry for delay! Our CIs have been broken for so long, and finally it's fixed now. |
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