mvcc: return -1 for wrong watcher range key >= end #6820
Conversation
| @@ -108,6 +109,10 @@ func (ws *watchStream) Watch(key, end []byte, startRev int64, fcs ...FilterFunc) | |||
| id := ws.nextID | |||
| ws.nextID++ | |||
|
|
|||
| // prevent wrong range where key >= end lexicographically | |||
There was a problem hiding this comment.
check before acquiring the lock?
| runWatchTest(t, testWatchWrongRange) | ||
| } | ||
|
|
||
| func testWatchWrongRange(t *testing.T, wctx *watchctx) { |
There was a problem hiding this comment.
should this test be in /integration instead of /clientv3/integration?
|
No strong opinion about it being translated in clientv3/etcdctl. We should probably teach etcdctl about watchresponse Err() regardless. |
|
All fixed. Think error PTAL. Thanks. |
| @@ -329,7 +329,7 @@ put key2 "some extra key" | |||
|
|
|||
| ### WATCH [options] [key or prefix] [range_end] | |||
|
|
|||
| Watch watches events stream on keys or prefixes, [key or prefix, range_end) if `range-end` is given. The watch command runs until it encounters an error or is terminated by the user. | |||
| Watch watches events stream on keys or prefixes, [key or prefix, range_end) if `range-end` is given. The watch command runs until it encounters an error or is terminated by the user. If range_end is given, it must be lexicographically greater than key. | |||
There was a problem hiding this comment.
greater than the key or "\x00".? there's a special case for a string with only "\x00" (see WithFromKey)
| @@ -36,7 +36,7 @@ var ( | |||
| func NewWatchCommand() *cobra.Command { | |||
| cmd := &cobra.Command{ | |||
| Use: "watch [options] [key or prefix] [range_end]", | |||
| Short: "Watches events stream on keys or prefixes", | |||
| Short: "Watches events stream on keys or prefixes (range_end is optional or must be lexicographically greater than key)", | |||
There was a problem hiding this comment.
this is sort of wordy, probably can drop
| @@ -99,6 +100,11 @@ type watchStream struct { | |||
| // Watch creates a new watcher in the stream and returns its WatchID. | |||
| // TODO: return error if ws is closed? | |||
| func (ws *watchStream) Watch(key, end []byte, startRev int64, fcs ...FilterFunc) WatchID { | |||
| // prevent wrong range where key >= end lexicographically | |||
| if len(end) != 0 && bytes.Compare(key, end) != -1 { | |||
There was a problem hiding this comment.
does this break Watch(ctx, "abc", clientv3.WithFromKey())?
|
@heyitsanthony Added special case handling for |
| @@ -99,6 +100,12 @@ type watchStream struct { | |||
| // Watch creates a new watcher in the stream and returns its WatchID. | |||
| // TODO: return error if ws is closed? | |||
| func (ws *watchStream) Watch(key, end []byte, startRev int64, fcs ...FilterFunc) WatchID { | |||
| // prevent wrong range where key >= end lexicographically | |||
| // except special case for watch from key '\x00' | |||
| if len(end) != 0 && !bytes.Equal(end, []byte{0}) && bytes.Compare(key, end) != -1 { | |||
There was a problem hiding this comment.
if len(end) != 0 && bytes.Compare(key, end) != -1 && !bytes.Equal(end, []byte{0})? common path is going to be key < end, right?
| t.Fatalf("key > end range given; id expected -1, got %d", id) | ||
| } | ||
| // in case client sends 'WithFromKey' request with smallest bytes range end | ||
| if id := w.Watch([]byte("foo"), []byte{}, 1); id != 0 { |
There was a problem hiding this comment.
should this be []byte{0}? I think []byte{} will give the same result as end == nil once passed through grpc?
|
@heyitsanthony I removed It's just empty bytes when passed to mvcc. |
|
@gyuho OK. Is that WithFromKey test correct, though? I think it still needs to be |
gyuho
force-pushed
the
watcher
branch
2 times, most recently
from
9fba1f7
to
1db58fb
Compare
|
@heyitsanthony Did you mean I added |
|
Oh oops, you're right. What was I thinking. lgtm |
gyuho
force-pushed
the
watcher
branch
2 times, most recently
from
d84c768
to
29246e8
Compare
|
Thanks merging in greens. |
Fix #6819.
@heyitsanthony @xiang90 Should we translate the error in etcdctl or clientv3 side? Currently clients will just see the watch create request is canceled.