Lock module support about python-etcd HOT 18 CLOSED

Not sure how the syntax with the "with" keyword (which I like as it's very pythonic, BTW) interacts with the other one. I'd probably adhere as much as possible to the syntax of pylockfile, which is IMO a good API:

http://pythonhosted.org/lockfile/lockfile.html

from python-etcd.

BTW, I'll wait for your work on locks; I won't start working on that.

from python-etcd.

I should mention acquire_lock and lock are two different ways of getting a lock, the user wouldn't use both.

I'll move to a class-based thing similar to pylockfile. Thanks for the feedback!

from python-etcd.

Actually, I take that second part back. I think keeping the locking methods in the Client class would be best, to keep things simple. Happy to change up method names to be similar to pylockfile, though.

from python-etcd.

IMHO we need to separate these modules from the main client implementation, or we'll end up having a huge class with tons of methods, mixing both low level methods (write, read) with high level primitives (locks, leader election). I've been looking for other similar libraries, mainly at the kazoo zookeper library (I was advised to keep off the main apache python library for zookeper), and they implement these primitives in separate modules (in the zookeper's argot "recipes").

So, (ripped from http://kazoo.readthedocs.org/en/latest/api/recipe/lock.html):

zk = KazooClient()
lock = zk.Lock("/lockpath", "my-identifier")
with lock:  # blocks waiting for lock acquisition
    # do something with the lock

Could be translated to our library in:

client = etcd.Client()
lock = etcd.Lock('/customer1', ttl=60)
lock.acquire()

pros for this syntax:

• clearer separation between low and high level ops
• less methods in the client class

cons:

• an additional call for every lock
• still unclear if this

client = etcd.Client()
lock = etcd.Lock('/customer1', ttl=60)
lock.acquire()

is better than

client = etcd.Client()
lock = etcd.Lock()
lock.acquire('/customer1', ttl=60)

(I think I prefer the first option but...)

As @lavagetto I also really like the context manager syntax but probably we'll also need to allow this:

client = etcd.Client()
lock = etcd.Lock('/customer1', ttl=60)
with lock as my_lock:
    print my_lock.index
    my_lock.renew()
    my_lock.value
    my_lock.renew()

allowing the consumer to retrieve data from the context manager.

This (I guess) answers your main question (returning an EtcdLock instead a simple value) as to make this possible you'd need to return a class.

I also prefer the pylockfile method names (release instead of unlock) but it's really a matter of taste.

from python-etcd.

@jplana ok, makes sense to me. I like the API you've presented.

The only thing that I think will need to change, is that I think we'll need to pass the client into Lock(), so it has access to the underlying methods (e.g. api_execute). Other than that, looks good to me.

I'll send a PR over soon.

from python-etcd.

A thought I sort of like:

client = etcd.Client()
lock_manager = etcd.LockManager(client)

with lock_manager.lock('/foo', ttl=60) as my_lock:
    time.sleep(120)
    # my_lock is still held, the lock manager uses a daemon thread to keep it up to date

The main "innovation" there being use of a daemon thread to keep locks alive when used as a context manager, instead of having to do it manually. In this case, sharing a lock manager between a lot of locks is nice so you don't have to have a thread per lock (although you do have to worry about having more locks than the thread can keep up with).

from python-etcd.

About separating the lock/leader methods: etcd devs are designing AGAINST the Zookeeper model, where every client library needs to implement its own model.

Boiling down to the basics, we have a few methods:

• read
• write
• delete

So I think a sensible syntax could be:

c = etcd.Client()
l = c.lock()
l.acquire('/lockname',...)
l.release('/lockname')
c.is_locked('/lockname')
c.break_lock('/lockname', ...) #This may be not implementable

And simply make etcd.Client.lock() a factory for etcd.Lock().

For the api_execute part, just pass the client as an argument to the etcd.Lock.__init__ method, and use api_execute from there.

Agree with @jplana on the context manager idea.

from python-etcd.

@russellhaering you can set locks with a TTL of 0 in etcd to make them permanent. We don't need to reimplement that in the client.

from python-etcd.

@russellhaering I'd try to avoid getting threads involved in the client as much as possible. Lets give the final library user the choice to use threads, "green-threads", processes, or whatever they choose to keep the lock.

from python-etcd.

"you can set locks with a TTL of 0 in etcd to make them permanent"

Good to know. They should document that.

from python-etcd.

On 30/12/13 22:35, Jim Rollenhagen wrote:

"you can set locks with a TTL of 0 in etcd to make them permanent"

Good to know. They should document that.

Yes, there is a few undocumented edges around etcd. I'm mixing docs and
code to understand how the thing works :)

G.

from python-etcd.

@lavagetto I'm not saying that we should keep the zookeper model, the kazoo library was just an example, however, why mixing both kinds of operations in the very same client? When we add the leader election should we add methods to the Client class to support that? What if then add barriers or any other additional mod? This could grow endlessly...

Etcd devs might not like the zookeper model but they've already separated the entry-points for mods under "/mod". Keeping these locks, leader elections and whatever comes next separated from read, write, delete makes sense (following their own exposed API).

As for the factory, something like:

class EtcdLock(object):
    def __init__(self, client, args):
        self._client = client
        self._args = args

class Client(object):

    def Lock(self, args=None):
        return EtcdLock(self, args)

would probably do.

I'm happy to move around the params passed (args passed to the factory vs args passed directly to the acquire()) however it would probably create some discrepancies for the client that uses the context manager vs calls the methods directly, for example:

# With args passed to the factory:

# Context manager 
client = etcd.Client()
with etcd.Lock('/customer1', ttl=60) as my_lock:
    print my_lock.index
    my_lock.renew()
    my_lock.value
    my_lock.renew()

# direct method calls
client = etcd.Client()
lock = etcd.Lock('/customer1', ttl=60)
lock.acquire()
print lock.value
lock.renew()

# With args passed to the lock methods

# Context manager 
client = etcd.Client()
# I think you can't avoid this, as you need to pass the params to lock
with etcd.Lock('/customer1', ttl=60) as my_lock:
    print my_lock.index
    my_lock.renew()
    my_lock.value
    my_lock.renew()

# direct method calls
client = etcd.Client()
lock = etcd.Lock()
lock.acquire('/customer1', ttl=60)
print lock(/customer1', ttl=60).value
lock.renew(/customer1', ttl=60)

It's rather...strange that depending on the way you use it you need to pass params or not. (But please correct me if I'm wrong with this)

from python-etcd.

@jplana you're not wrong.

My idea was something like:

l = etcd.Client().lock()
#equivalent
c = etcd.Client()
l = etcd.Lock(c)
#context manager
with l.acquire('/customer1', 60) as lock:
    ...
    lock.renew(60) 

#normal use
l.acquire('/customer1', 60)
...

l.renew()
l.release()

So the API is more consistent. Do you think this is a good option?

from python-etcd.

I was thinking the opposite, with passing key/ttl/value into the class itself. The main reason being that the context manager version of acquire and the "normal" version of acquire would need different names.

Something like:

l = etcd.Client().get_lock('/customer1', 60)
#equivalent
c = etcd.Client()
l = etcd.Lock(c, '/customer1', 60)
#context manager
with l as lock:
    ...
    lock.renew(60) 

#normal use
l.acquire()
...

l.renew()
l.release()

I think this is simpler, and renaming client.lock to client.get_lock minimizes confusion (although the name still could imply the lock is acquired there and could be better).

from python-etcd.

@jimrollenhagen good, I like it! A minor correction: the context manager is useful if you create the lock in the context itself:

with etcd.Client().get_lock('/customer1', 60) as lock:
    ...
    lock.renew(60)

Cannot wait to look at your work :)

from python-etcd.

@lavagetto me too! 👍

from python-etcd.

Glad to hear it!

I've opened the PR on #19. Going to close this issue out.

from python-etcd.