txn support IS_EXIST and NOT_EXIST compare #10769
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10769 +/- ##
==========================================
+ Coverage 62.47% 63.12% +0.64%
==========================================
Files 392 392
Lines 37158 37175 +17
==========================================
+ Hits 23215 23466 +251
+ Misses 12381 12132 -249
- Partials 1562 1577 +15
Continue to review full report at Codecov.
|
|
wow, awesome! I'd recommend we add a test with this patch to keep it stable! Also, do we want to guarantee the behaviour of checking if a value when that value doesn't exist? Should it be an error instead? |
|
Could you provide an example usage? Is this trying to decide whether a key exist? I am trying to understand the difference between this and something like: |
|
@jingyih yes, this trying to decide whether a key exist or not exist. I think |
| ctx, cancel := context.WithTimeout(context.Background(), requestTimeout) | ||
| _, err = kvc.Txn(ctx). | ||
| // txn value comparisons are lexical | ||
| If(clientv3.CompareExist(clientv3.Value("key"), "ne")). |
There was a problem hiding this comment.
Sorry I am still confused. If we replace this line with the following line, isn't it the same?
If(clientv3.Compare(clientv3.ModRevision("key"), "=", 0))
There was a problem hiding this comment.
BTW, I don't think this PR solves the original issue #10566. The original issue is that when we use clientv3.Compare(clientv3.Value(key), "=", value), the compare always return false if key does not exist, regardless of "=" or "!=".
There was a problem hiding this comment.
Thank you for your suggestions.
1: There are the same, i just think ModRevision make user realize this API should be use as Optimistic lock in mvcc storage
2: clientv3.Compare(clientv3.Value(key), "=", value) always return false, the reason just is
Line 449 in dc6885d
This PR had delete these codes.
I am also confused why add these codes, and think value compare result should be decided by bytes.Compare
There was a problem hiding this comment.
Thank you for your suggestions.
1: There are the same, i just think
ModRevisionmake user realize this API should be use as Optimistic lock in mvcc storage
Thanks for confirming. I think we should not break API to implement a feature that already exists.
2:
clientv3.Compare(clientv3.Value(key), "=", value)always return false, the reason just isLine 449 in dc6885d
This PR had delete these codes.
I am also confused why add these codes, and think value compare result should be decided by bytes.Compare
Sorry I missed the deleting part. Removing the code block means treating non-exist-key's value as an empty string (or its byte presentation). This is probably fine, but we should more carefully think through this.
|
So there's this: https://godoc.org/github.com/coreos/etcd/clientv3/clientv3util Secondly, I think this relates to the whole design of what should happen if you try to read the value of a key that doesn't exist. I think this should propagate upwards as an error. This is the equivalent of a nil in golang. I can explain this better if it would help. |
|
Wow, I will close this PR |
|
@purpleidea Thanks for the link. I agree the behavior should be carefully designed. The discussion can be continued in the original issue. |
|
@yylt Thanks for trying to fix this! |
|
@yylt I think there was some promise in the PR, in particular when I previously read through the code, I noticed the: https://github.com/etcd-io/etcd/pull/10769/files#diff-f141df18bc5c4d2e821b7d9dfb484f95L449 which you removed was probably a bug which was masking weird key not exists handling as discussed in #10566 Perhaps you'd like to take what you learned digging through this part of the code and look at #10571 ? |
|
@purpleidea yes, I can try the feature #10571 |
txn support IS_EXIST and NOT_EXIST compare
Fix #10566
cc @purpleidea