-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
api: txn comparisons on ranges #8025
Conversation
28c1e4b
to
943d6b7
Compare
Codecov Report
@@ Coverage Diff @@
## master #8025 +/- ##
==========================================
- Coverage 76.49% 76.41% -0.09%
==========================================
Files 342 342
Lines 26549 26561 +12
==========================================
- Hits 20309 20296 -13
- Misses 4785 4800 +15
- Partials 1455 1465 +10
Continue to review full report at Codecov.
|
943d6b7
to
2256599
Compare
etcdserver/etcdserverpb/rpc.proto
Outdated
@@ -510,6 +510,9 @@ message Compare { | |||
// value is the value of the given key, in bytes. | |||
bytes value = 7; | |||
} | |||
// range_end compares over the range [key, range_end). See RangeRequest. | |||
bytes range_end = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when range_end is given, all the keys within the range will be compared against the same target? Probably we should make the comment more clear?
Also can you give me an example use case of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's the same target; will clarify comment
An example use case from the leasing kv: delete range. To safely delete a range of keys the leasing kv must ensure no other client holds a key lease (represented as a "leasing key" controlled by the leasing protocol) on any key in the range. The leasing kv can acquire/revoke key leases such that so long as there are no new key leases acquired on a range, that range is safe to delete. This check can be done with a ranged comparison over the corresponding leasing keys and testing CreateRev < safeToDeleteRev
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i can see people might want multiple targets eventually. but it is something easy to support i feel.
2256599
to
1749626
Compare
@@ -99,6 +99,18 @@ func (cmp *Cmp) ValueBytes() []byte { | |||
// WithValueBytes sets the byte slice for the comparison's value. | |||
func (cmp *Cmp) WithValueBytes(v []byte) { cmp.TargetUnion.(*pb.Compare_Value).Value = v } | |||
|
|||
// WithRange sets the comparison to scan the range [key, end). | |||
func (cmp Cmp) WithRange(end string) Cmp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(cmp *Cmp)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it that way but clientv3.Compare(...).WithRange(...)
will complain that the result from Compare()
is not a pointer and it doesn't seem worth changing Compare()'s return value to a pointer to support this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it returns Cmp
anyway. Nvm.
} | ||
|
||
// WithPrefix sets the comparison to scan all keys prefixed by the key. | ||
func (cmp Cmp) WithPrefix() Cmp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(cmp *Cmp)
?
1749626
to
8f34d0c
Compare
lgtm |
proxy tests failing on fixed grpc data race; merging |
Server and client support for txn comparisons over ranges.
Fixes #7924