grpc lock service #7444

heyitsanthony · 2017-03-07T22:15:17Z

xiang90 · 2017-03-07T23:25:26Z

etcdserver/api/v3lock/v3lockpb/v3lock.proto

+        body: "*"
+    };
+  }
+  rpc Unlock(UnlockRequest) returns (UnlockResponse) {


Can we add a verify or check interface? So user can verify if itself is still the locker owner or the other component is still the owner of the lock?

The key will only be returned on lock acquire. So long as Key exists with create rev <= response.header.revision (for most cases you'll only need the exists check), it holds the lock. That way it can interact with Txns safely.

I think it only needs some documentation, not a full RPC.

lets say we want to a lock with name mylock. the returned the key will be mylock/001. Then the client can check it mylock/abc still exists to see if the lock is still held by the owner? we have the guarantee that lock key is unique?

It's keyed by lease so it will be unique for distinct leases.

ok. this should work. i agree it is just a doc issue. client can build utility func to provide a better interface to end users though.

xiang90 · 2017-03-08T04:44:45Z

etcdserver/api/v3lock/lock.go

+	if err != nil {
+		return nil, err
+	}
+	s.Orphan()


why do we need to call Orphan here?

Otherwise the server will keep the lease alive indefinitely.

An alternative to putting it here is defer s.Orphan() so it'll keep the lease alive until the the lock is acquired so the client doesn't have to worry about keep alives. The problem with that is there's no guarantee how much time the lease will have left after the acquire. This could be solved by issuing a keepalive before returning from the RPC.

Another option is to make leases optional and have an additional TTL field so the server can grant a lease and return a lock held for at least the given TTL.

xiang90 · 2017-03-08T04:45:56Z

etcdserver/api/v3client/v3client.go

+)
+
+func New(s *etcdserver.EtcdServer) *clientv3.Client {
+	c := clientv3.NewCtxClient(context.TODO())


use context.Background instead?

xiang90 · 2017-03-08T04:47:41Z

clientv3/client.go

@@ -77,6 +77,11 @@ func New(cfg Config) (*Client, error) {
 	return newClient(&cfg)
 }

+func NewCtxClient(ctx context.Context) *Client {


Add a comment on this Client? Probably document that this client is defined for internal usage?

xiang90 · 2017-03-08T05:06:49Z

etcdserver/api/v3lock/v3lockpb/v3lock.proto

+option (gogoproto.marshaler_all) = true;
+option (gogoproto.unmarshaler_all) = true;
+
+service Lock {


we need to document this proto file (service + messages)

xiang90 · 2017-03-08T23:45:59Z

LGTM in general. @fanminshi please take a look.

heyitsanthony · 2017-03-10T17:14:25Z

Had to slip in a slight grpcproxy restructure to break a cyclic dependency introduced by the cluster autosyncing patch; the client<->server grpc adapters now live in proxy/grpcproxy/adapter.

fanminshi · 2017-03-10T18:25:59Z

clientv3/concurrency/mutex.go

 }

 func NewMutex(s *Session, pfx string) *Mutex {
-	return &Mutex{s, pfx + "/", "", -1}
+	return &Mutex{s, pfx + "/", "", -1, nil}
 }

 // Lock locks the mutex with a cancellable context. If the context is cancelled


cancellable => cancelable?

This isn't part of the patch, but OK...

fanminshi · 2017-03-10T21:47:23Z

clientv3/concurrency/key.go

 	"github.com/coreos/etcd/mvcc/mvccpb"
 	"golang.org/x/net/context"
 )

-func waitDelete(ctx context.Context, client *v3.Client, key string, rev int64) error {
+func waitDelete(ctx context.Context, client *v3.Client, key string, rev int64) (*pb.ResponseHeader, error) {


(*pb.ResponseHeader is not used by waitDeletes(), and it seems like it's not used by anything else. Am I missing something?

yeah this can be reverted

fanminshi · 2017-03-10T21:50:31Z

clientv3/concurrency/key.go

 }

 // waitDeletes efficiently waits until all keys matching the prefix and no greater
 // than the create revision.
-func waitDeletes(ctx context.Context, client *v3.Client, pfx string, maxCreateRev int64) error {
+func waitDeletes(ctx context.Context, client *v3.Client, pfx string, maxCreateRev int64) (*pb.ResponseHeader, error) {
 	getOpts := append(v3.WithLastCreate(), v3.WithMaxCreateRev(maxCreateRev))
 	for {


Could you explain what's the rationale of for loop here? Thanks.

It loops because waitDelete will return after sensing one delete. There is no guarantee that all keys within the revision range will be deleted by that time. This code is a little old and the API is a bit stronger now so this can possibly be optimized to be single-shot on the watcher, but I don't want to change it for this patch.

fanminshi · 2017-03-10T22:00:00Z

clientv3/concurrency/key.go

 		}
 		lastKey := string(resp.Kvs[0].Key)
-		if err = waitDelete(ctx, client, lastKey, resp.Header.Revision); err != nil {
-			return err
+		if _, werr := waitDelete(ctx, client, lastKey, resp.Header.Revision); werr != nil {


is the resp.Header.Revision here corresponding revision when lastKey is created? or the revision when Get() request is finished?

It's the revision the Get sees. The create revision for lastKey is already in Kvs[0].

codecov-io · 2017-03-11T01:17:20Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5193965). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #7444   +/-   ##
=========================================
  Coverage          ?   71.12%           
=========================================
  Files             ?      241           
  Lines             ?    21347           
  Branches          ?        0           
=========================================
  Hits              ?    15184           
  Misses            ?     5033           
  Partials          ?     1130

Impacted Files	Coverage Δ
embed/serve.go	`60.68% <100%> (ø)`
clientv3/client.go	`78.37% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5193965...300323f. Read the comment docs.

xiang90 · 2017-03-13T21:56:43Z

etcdserver/api/v3lock/v3lockpb/v3lock.proto

+message LockResponse {
+  etcdserverpb.ResponseHeader header = 1;
+  // key is a key that will exist on etcd for the duration
+  // that the Lock caller owns the lock.


add a warning that: users should not modify this key with kv api, or lock might have undefined behavior?

xiang90 · 2017-03-13T21:57:28Z

LGTM

fanminshi · 2017-03-13T22:01:48Z

lgtm. Thanks!

Break cyclic dependency: clientv3/naming <-> integration <-> v3client <-> grpcproxy <-> clientv3/naming

For creating client from etcdserver.

xiang90 reviewed Mar 7, 2017

View reviewed changes

xiang90 reviewed Mar 8, 2017

View reviewed changes

gyuho added the kind/release-note label Mar 8, 2017

heyitsanthony force-pushed the lock-service branch 3 times, most recently from 553d8c2 to 62d653d Compare March 10, 2017 05:29

fanminshi reviewed Mar 10, 2017

View reviewed changes

heyitsanthony force-pushed the lock-service branch from 62d653d to eaefc33 Compare March 10, 2017 22:15

xiang90 reviewed Mar 13, 2017

View reviewed changes

heyitsanthony added 8 commits March 13, 2017 15:23

adapter, integration: split out grpc adapters from grpcproxy package

b025cdd

Break cyclic dependency: clientv3/naming <-> integration <-> v3client <-> grpcproxy <-> clientv3/naming

clientv3: permit creating client without grpc connection

a2cdd90

For creating client from etcdserver.

concurrency: support returning response header for mutex

d51c8bb

v3client: a bridge between an etcdserver and a clientv3

763a37d

v3lock: server-side api for locking

47cd9d0

scripts: generate lock service rpc stubs

c737bf3

embed: serve lock api

ad1a790

integration: test grpc lock service

300323f

heyitsanthony force-pushed the lock-service branch from eaefc33 to 300323f Compare March 13, 2017 22:23

heyitsanthony merged commit 1a6be70 into etcd-io:master Mar 14, 2017

heyitsanthony deleted the lock-service branch March 14, 2017 07:01

heyitsanthony mentioned this pull request Mar 14, 2017

clientv3: embedded client #4709

Closed

gyuho mentioned this pull request Mar 14, 2017

use etcd's embedded clients etcd-io/etcdlabs#201

Closed

This was referenced Apr 15, 2017

Lock with leases kragniz/python-etcd3#109

Closed

Add distributed locking capability microsoft/etcd3#5

Merged

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

grpc lock service #7444

grpc lock service #7444

heyitsanthony commented Mar 7, 2017

xiang90 Mar 7, 2017

heyitsanthony Mar 7, 2017

xiang90 Mar 8, 2017

heyitsanthony Mar 8, 2017

xiang90 Mar 8, 2017

xiang90 Mar 8, 2017

heyitsanthony Mar 9, 2017

xiang90 Mar 8, 2017

xiang90 Mar 8, 2017

xiang90 Mar 8, 2017

xiang90 commented Mar 8, 2017

heyitsanthony commented Mar 10, 2017

fanminshi Mar 10, 2017

heyitsanthony Mar 10, 2017

fanminshi Mar 10, 2017

heyitsanthony Mar 10, 2017

fanminshi Mar 10, 2017

heyitsanthony Mar 10, 2017

fanminshi Mar 10, 2017

heyitsanthony Mar 10, 2017

codecov-io commented Mar 11, 2017 •

edited

Loading

xiang90 Mar 13, 2017 •

edited

Loading

xiang90 commented Mar 13, 2017

fanminshi commented Mar 13, 2017

grpc lock service #7444

grpc lock service #7444

Conversation

heyitsanthony commented Mar 7, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Mar 8, 2017

heyitsanthony commented Mar 10, 2017

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

codecov-io commented Mar 11, 2017 • edited Loading

Codecov Report

xiang90 Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

xiang90 commented Mar 13, 2017

fanminshi commented Mar 13, 2017

codecov-io commented Mar 11, 2017 •

edited

Loading

xiang90 Mar 13, 2017 •

edited

Loading