Skip to content
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.

Sign up for GitHub

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

txn support IS_EXIST and NOT_EXIST compare #10769

Closed
wants to merge 3 commits into from
Closed

txn support IS_EXIST and NOT_EXIST compare #10769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07b095d
add IS_EXIST and NOT_EXIST compare
May 29, 2019
d38394f
gofmt and add proto
yylt May 29, 2019
0d113f8
add notexist example
May 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions clientv3/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ func Compare(cmp Cmp, result string, v interface{}) Cmp {
r = pb.Compare_GREATER
case "<":
r = pb.Compare_LESS
case "ie":
r = pb.Compare_IS_EXIST
case "ne":
r = pb.Compare_NOT_EXIST
default:
panic("Unknown result op")
}
Expand All @@ -68,6 +72,21 @@ func Compare(cmp Cmp, result string, v interface{}) Cmp {
return cmp
}

func CompareExist(cmp Cmp, result string) Cmp {
var r pb.Compare_CompareResult

switch result {
case "ie":
r = pb.Compare_IS_EXIST
case "ne":
r = pb.Compare_NOT_EXIST
default:
panic("Unknown result op")
}
cmp.Result = r
return cmp
}

func Value(key string) Cmp {
return Cmp{Key: []byte(key), Target: pb.Compare_VALUE}
}
Expand Down
37 changes: 37 additions & 0 deletions clientv3/example_kv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,43 @@ func ExampleKV_txn() {
// Output: key : XYZ
}

func ExampleKV_txn_notexist() {
cli, err := clientv3.New(clientv3.Config{
Endpoints: endpoints,
DialTimeout: dialTimeout,
})
if err != nil {
log.Fatal(err)
}
defer cli.Close()

kvc := clientv3.NewKV(cli)

ctx, cancel := context.WithTimeout(context.Background(), requestTimeout)
_, err = kvc.Txn(ctx).
// txn value comparisons are lexical
If(clientv3.CompareExist(clientv3.Value("key"), "ne")).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 "!=".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

etcd/etcdserver/apply.go

Line 449 in dc6885d

if c.Target == pb.Compare_VALUE {

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 is

etcd/etcdserver/apply.go

Line 449 in dc6885d

if c.Target == pb.Compare_VALUE {

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.

// the "Then" runs, since "xyz" > "abc"
Then(clientv3.OpPut("key", "XYZ")).
// the "Else" does not run
Else(clientv3.OpDelete("key" )).
Commit()
cancel()
if err != nil {
log.Fatal(err)
}

gresp, err := kvc.Get(context.TODO(), "key")
cancel()
if err != nil {
log.Fatal(err)
}
for _, ev := range gresp.Kvs {
fmt.Printf("%s : %s\n", ev.Key, ev.Value)
}
// Output: key : XYZ
}

func ExampleKV_do() {
cli, err := clientv3.New(clientv3.Config{
Endpoints: endpoints,
Expand Down
9 changes: 4 additions & 5 deletions etcdserver/apply.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,6 @@ func applyCompare(rv mvcc.ReadView, c *pb.Compare) bool {
return false
}
if len(rr.KVs) == 0 {
if c.Target == pb.Compare_VALUE {
// Always fail if comparing a value on a key/keys that doesn't exist;
// nil == empty string in grpc; no way to represent missing value
return false
}
return compareKV(c, mvccpb.KeyValue{})
}
for _, kv := range rr.KVs {
Expand Down Expand Up @@ -501,6 +496,10 @@ func compareKV(c *pb.Compare, ckv mvccpb.KeyValue) bool {
return result > 0
case pb.Compare_LESS:
return result < 0
case pb.Compare_IS_EXIST:
return ckv.Value != nil
case pb.Compare_NOT_EXIST:
return ckv.Value == nil
}
return true
}
Expand Down
Loading