*: 'ignore_value' to detach lease with PutRequest #6894

gyuho · 2016-11-23T07:02:09Z

gyuho · 2016-11-23T15:56:04Z

@xiang90 Please review. Thanks.

xiang90 · 2016-11-23T19:35:01Z

I think @sinsharat is working on the similar thing. Can you look through this?

sinsharat · 2016-11-24T04:22:06Z

@gyuho thanks for working on this as i am stuck with travel. Please give me some time to review this as i am still on travel for another 4days and don't have my laptop or internet connection with me to review it properly.
Thanks!

sinsharat · 2016-11-30T19:16:43Z

Documentation/dev-guide/api_reference_v3.md

@@ -616,6 +616,7 @@ Empty field.
 | value | value is the value, in bytes, to associate with the key in the key-value store. | bytes |
 | lease | lease is the lease ID to associate with the key in the key-value store. A lease value of 0 indicates no lease. | int64 |
 | prev_kv | If prev_kv is set, etcd gets the previous key-value pair before changing it. The previous key-value pair will be returned in the put response. | bool |
+| preserve_value | If preserve_value is set, etcd overwrites the key with previous value. Use this to detach a lease by overwriting the key. | bool |


If preserve_value is set, etcd preserves the key by detaching or replacing lease if any associated with it. Use this to detach the lease or replace lease for the key.?

heyitsanthony · 2016-11-30T19:22:13Z

missing grpcproxy support?

sinsharat · 2016-11-30T19:29:24Z

etcdctl/README.md

@@ -43,6 +43,8 @@ PUT assigns the specified value with the specified key. If key already holds a v

 - lease -- lease ID (in hexadecimal) to attach to the key.

+- preserve-value -- allow empty value to overwrite key with previous value.


detatch or replace lease for key-value pair?

sinsharat · 2016-11-30T19:38:20Z

etcdctl/ctlv3/command/put_command.go

@@ -51,6 +53,7 @@ will store the content of the file to <key>.
 	}
 	cmd.Flags().StringVar(&leaseStr, "lease", "0", "lease ID (in hexadecimal) to attach to the key")
 	cmd.Flags().BoolVar(&putPrevKV, "prev-kv", false, "return the previous key-value pair before modification")
+	cmd.Flags().BoolVar(&putPreserveVal, "preserve-value", false, "allow empty value to overwrite key with previous value")


detach lease if any for the key to preserve it.?

sinsharat · 2016-11-30T19:43:56Z

i am not convinced about putting a new key with an empty value when preserve-value is passed and the key doesn't exist. I feel it should return an error instead, since the main purpose of this flag is to detach lease for an existing key without passing value to it.

gyuho · 2016-11-30T19:51:40Z

i am not convinced about putting a new key with an empty value when preserve-value is passed and the key doesn't exist. I feel it should return an error instead

@sinsharat Actually this PR returns error here a8104dd? But we need better documentation on that case.

@heyitsanthony I will work on grpc-proxy part as well. Thanks!

sinsharat · 2016-11-30T19:55:04Z

@gyuho if that's the case we probably need to work on correcting comments as well. Will give a deeper look to this.
Thanks!

sinsharat · 2016-12-01T18:57:21Z

@gyuho Do we support lease flag or prev kv along with preserver value? Have you tested it?

heyitsanthony · 2016-12-01T21:26:43Z

etcdserver/etcdserverpb/rpc.proto

+
+  // If preserve_value is set, etcd preserves the key by detaching or replacing
+  // lease if any. Use this to detach a lease or replace lease of the key.
+  bool preserve_value = 5;


maybe ignore_value (no_value?) so it's talking about value from the rpc?

Yeah ignore-value sounds clear. Will change.

heyitsanthony

needs some testing around case where the key doesn't exist for put and txn puts

heyitsanthony · 2016-12-01T21:33:12Z

etcdserver/etcdserverpb/rpc.proto

@@ -423,6 +423,10 @@ message PutRequest {
  // If prev_kv is set, etcd gets the previous key-value pair before changing it.
  // The previous key-value pair will be returned in the put response.
  bool prev_kv = 4;
+
+  // If preserve_value is set, etcd preserves the key by detaching or replacing


etcd updates the key using its current value. Returns an error if the key does not exist.

I think the docs can ignore the lease use case. There's been a few requests for a similar feature with leases-- modify the key but keep the lease the same. If we add preserve_lease in the future, then the preserve_value = true won't necessarily imply a lease attach/detach.

heyitsanthony · 2016-12-01T21:44:13Z

clientv3/integration/kv_test.go

@@ -113,6 +113,64 @@ func TestKVPut(t *testing.T) {
 	}
 }

+func TestKVPutDetachLease(t *testing.T) {


This does too much server testing for a client test. Checking that a Put WithPreserveValue doesn't clobber the old value should be OK.

heyitsanthony · 2016-12-01T21:48:50Z

integration/v3_grpc_test.go

+	clus := NewClusterV3(t, &ClusterConfig{Size: 1})
+	defer clus.Terminate(t)
+
+	// create lease


does this need all the lease testing? it's not really a special case in the implementation. Can probably get away with only checking that the value isn't clobbered.

heyitsanthony · 2016-12-01T21:49:46Z

e2e/ctl_v3_kv_test.go

@@ -62,6 +63,35 @@ func putTest(cx ctlCtx) {
 	}
 }

+func putTestPreserveValue(cx ctlCtx) {
+	id, err := ctlV3LeaseGrant(cx, 30)


is the lease stuff necessary?

heyitsanthony · 2016-12-01T22:04:24Z

etcdserver/apply.go

@@ -175,6 +175,10 @@ func (a *applierV3backend) Put(txnID int64, p *pb.PutRequest) (*pb.PutResponse,
 		}
 	}

+	if p.PreserveValue && rr != nil && len(rr.KVs) != 0 {
+		p.Value = rr.KVs[0].Value
+	}


p.PreserveValue && (rr == nil || len(rr.Kvs) == 0) should be an error since it shouldn't create a new key. Txns will need a special case to check that all keys exist before doing the puts.

heyitsanthony · 2016-12-01T22:11:56Z

etcdserver/api/v3rpc/rpctypes/error.go

-	ErrGRPCFutureRev    = grpc.Errorf(codes.OutOfRange, "etcdserver: mvcc: required revision is a future revision")
-	ErrGRPCNoSpace      = grpc.Errorf(codes.ResourceExhausted, "etcdserver: mvcc: database space exceeded")
+	ErrGRPCEmptyKey      = grpc.Errorf(codes.InvalidArgument, "etcdserver: key is not provided")
+	ErrGRPCNotEmptyValue = grpc.Errorf(codes.InvalidArgument, "etcdserver: value is provided")


maybe ErrGRPCValue?

yeah will rework on these. Thanks!

gyuho · 2016-12-02T00:51:19Z

@heyitsanthony All addressed. PTAL. Thanks!

Please note that applyUnion method signature changed.

/cc @sinsharat

heyitsanthony · 2016-12-05T22:28:02Z

@gyuho yeah, shouldn't need to import etcdserver; the tests are already using rpctypes

gyuho · 2017-01-13T21:21:29Z

This has been reviewed. Just rebased on master branch. Merging in greens. Thanks.

xiang90 · 2017-01-13T21:23:54Z

@gyuho I put a release note on this.

gyuho · 2017-01-13T21:24:19Z

Ok ack (for v3.2)

gyuho force-pushed the preserve-value branch 2 times, most recently from f100bdf to f6ca32b Compare November 23, 2016 07:08

gyuho added the kind/release-note label Nov 23, 2016

sinsharat reviewed Nov 30, 2016

View reviewed changes

gyuho added the WIP label Nov 30, 2016

gyuho force-pushed the preserve-value branch from f6ca32b to ba9915d Compare December 1, 2016 00:12

gyuho removed the WIP label Dec 1, 2016

gyuho force-pushed the preserve-value branch 2 times, most recently from 5ae6001 to 2149698 Compare December 1, 2016 06:42

gyuho force-pushed the preserve-value branch from 2149698 to aaa5814 Compare December 1, 2016 19:30

gyuho added the WIP label Dec 1, 2016

gyuho force-pushed the preserve-value branch from aaa5814 to 961c868 Compare December 1, 2016 19:38

gyuho removed the WIP label Dec 1, 2016

heyitsanthony reviewed Dec 1, 2016

View reviewed changes

gyuho added the WIP label Dec 1, 2016

gyuho changed the title *: 'preserve_value' to detach lease with PutRequest *: 'no_value' to detach lease with PutRequest Dec 1, 2016

heyitsanthony suggested changes Dec 1, 2016

View reviewed changes

gyuho force-pushed the preserve-value branch from 961c868 to d0f567e Compare December 2, 2016 00:40

gyuho removed the WIP label Dec 2, 2016

gyuho removed the kind/release-note label Dec 5, 2016

gyuho added this to the v3.2.0 milestone Dec 5, 2016

gyuho force-pushed the preserve-value branch from ca6d096 to 681c1f8 Compare December 5, 2016 22:45

gyuho added the WIP label Dec 6, 2016

gyuho force-pushed the preserve-value branch 2 times, most recently from 7e82472 to 4d282de Compare December 9, 2016 03:33

gyuho force-pushed the preserve-value branch 3 times, most recently from 469dbd7 to 7ddc71f Compare December 16, 2016 21:53

heyitsanthony approved these changes Dec 30, 2016

View reviewed changes

gyuho force-pushed the preserve-value branch from 7ddc71f to 1d99f1c Compare January 13, 2017 21:02

gyuho removed the WIP label Jan 13, 2017

xiang90 added the kind/release-note label Jan 13, 2017

gyuho added 9 commits January 13, 2017 15:13

*: 'ignore_value' to detach lease with PutRequest

8eea939

*: regenerate proto files with 'ignore_value'

7f8b577

v3rpc: error for non-empty value with ignore_value

e655420

etcdserver: use prev-value for ignore_value writes

8752ee5

integration: test Put,Txn with ignore_value flag

a66f133

clientv3: add 'WithIgnoreValue' option

d94d221

ctlv3: add '--ignore-value' flag to 'put' command

e03850c

e2e: test put command with '--ignore-value' flag

5dffa38

grpcproxy: handle 'IgnoreValue' field in PutRequest

0f8060b

gyuho force-pushed the preserve-value branch from 1d99f1c to 0f8060b Compare January 13, 2017 23:13

gyuho merged commit 118fd18 into etcd-io:master Jan 14, 2017

gyuho deleted the preserve-value branch January 14, 2017 00:02

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: 'ignore_value' to detach lease with PutRequest #6894

*: 'ignore_value' to detach lease with PutRequest #6894

gyuho commented Nov 23, 2016

gyuho commented Nov 23, 2016

xiang90 commented Nov 23, 2016

sinsharat commented Nov 24, 2016

sinsharat Nov 30, 2016 •

edited

Loading

heyitsanthony commented Nov 30, 2016

sinsharat Nov 30, 2016

sinsharat Nov 30, 2016 •

edited

Loading

sinsharat commented Nov 30, 2016

gyuho commented Nov 30, 2016 •

edited

Loading

sinsharat commented Nov 30, 2016

sinsharat commented Dec 1, 2016 •

edited

Loading

heyitsanthony Dec 1, 2016 •

edited

Loading

gyuho Dec 1, 2016

heyitsanthony left a comment

heyitsanthony Dec 1, 2016

heyitsanthony Dec 1, 2016

heyitsanthony Dec 1, 2016

heyitsanthony Dec 1, 2016

heyitsanthony Dec 1, 2016

heyitsanthony Dec 1, 2016

gyuho Dec 1, 2016

gyuho commented Dec 2, 2016

heyitsanthony commented Dec 5, 2016

gyuho commented Jan 13, 2017

xiang90 commented Jan 13, 2017

gyuho commented Jan 13, 2017

		@@ -43,6 +43,8 @@ PUT assigns the specified value with the specified key. If key already holds a v

		- lease -- lease ID (in hexadecimal) to attach to the key.

		- preserve-value -- allow empty value to overwrite key with previous value.

*: 'ignore_value' to detach lease with PutRequest #6894

*: 'ignore_value' to detach lease with PutRequest #6894

Conversation

gyuho commented Nov 23, 2016

gyuho commented Nov 23, 2016

xiang90 commented Nov 23, 2016

sinsharat commented Nov 24, 2016

sinsharat Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

heyitsanthony commented Nov 30, 2016

Choose a reason for hiding this comment

sinsharat Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

sinsharat commented Nov 30, 2016

gyuho commented Nov 30, 2016 • edited Loading

sinsharat commented Nov 30, 2016

sinsharat commented Dec 1, 2016 • edited Loading

heyitsanthony Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyitsanthony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Dec 2, 2016

heyitsanthony commented Dec 5, 2016

gyuho commented Jan 13, 2017

xiang90 commented Jan 13, 2017

gyuho commented Jan 13, 2017

sinsharat Nov 30, 2016 •

edited

Loading

sinsharat Nov 30, 2016 •

edited

Loading

gyuho commented Nov 30, 2016 •

edited

Loading

sinsharat commented Dec 1, 2016 •

edited

Loading

heyitsanthony Dec 1, 2016 •

edited

Loading