*: combine Watch, WatchPrefix with variadic function #4602
Conversation
| rev int64 | ||
|
|
||
| // for watch | ||
| prefix string |
There was a problem hiding this comment.
I think this should use end instead of having a separate prefix field; watch can check the op for if end == nil || deepEqual(end, expectedPrefix) and throw an error if it's something else.
There was a problem hiding this comment.
Oh ok. Will fix. Thanks!
gyuho
force-pushed
the
watch_option
branch
5 times, most recently
from
93aa1e6
to
e95295d
Compare
|
/cc @heyitsanthony @xiang90 PTAL.
Thanks! |
| func opWatch(key string, opts ...OpOption) Op { | ||
| ret := Op{t: tRange, key: []byte(key)} | ||
| ret.applyOpts(opts) | ||
| switch { |
There was a problem hiding this comment.
can you pull getPrefix([]byte) []byte out of WithPrefix() so this can check
case ret.end != nil && reflect.DeepEqual(end, getPrefix(ret.key)): panic("only supports single keys or prefixes")?
There was a problem hiding this comment.
Sure. Will add that. Thanks.
|
lgtm aside from the missing prefix checking, thanks! |
|
@heyitsanthony Just added that function and check. Thanks! Will merge after green lights. |
|
test failed. will take a look. |
| ret.applyOpts(opts) | ||
| switch { | ||
| case ret.end != nil && !reflect.DeepEqual(ret.end, getPrefix(ret.key)): | ||
| panic("only supports single keys or prefixes") |
There was a problem hiding this comment.
@heyitsanthony Could you check this part? ret.end != nil && !reflect.DeepEqual(ret.end, getPrefix(ret.key))?
So if someone specifies WithPrefix in Watch api, end will be non nil. And it should be equal to getPrefix(ret.key). So I think it should be !reflect.DeepEqual(ret.end, getPrefix(ret.key)), not reflect.DeepEqual(ret.end, getPrefix(ret.key)). Test failed with case ret.end != nil && reflect.DeepEqual(end, getPrefix(ret.key)).
Thanks.
There was a problem hiding this comment.
Yes, you're right. I accidentally dropped the !.
There was a problem hiding this comment.
Thanks! Merging since tests passed.
*: combine Watch, WatchPrefix with variadic function
For #4598.