Managed Connections for txzk

Managed Connections for txzk

A managed connection automatically handles transient connection failures,
and session expiration. On session expiration it recreates ephemeral nodes,
and fire watch events on extant watchers.

https://code.launchpad.net/~hazmat/txzookeeper/managed-watch-and-ephemeral/+merge/100715

(do not edit description out of merge proposal)

[hazmat, 2012-04-04 00:55:03]

Please take a look.

[fwereade, 2012-04-04 13:23:29]

This looks very nice indeed, but the vagueness of the "comes at a cost" comment
makes me wonder what I'm missing...

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py
File txzookeeper/managed.py (right):

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode66
txzookeeper/managed.py:66: mgr  # keep the flakes happy
Can't you just do:

  with self._ctx():

?

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode70
txzookeeper/managed.py:70: mgr  # keep the flakes happy
Ditto

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode122
txzookeeper/managed.py:122: come at a cost though.
Please expand ;).

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode138
txzookeeper/managed.py:138: """Called on intercept of session expiration to
restablish the session.
I'm pretty sure there are 2 es in re-establish, and that the hyphen may have
fallen out of common use but remains IMO more readable. As you wish.

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode140
txzookeeper/managed.py:140: This will reconnect to zk, restabslish ephemerals,
and trigger watches.
I like the word "restabslish", but ditto.

https://codereview.appspot.com/5976074/diff/1/txzookeeper/tests/test_managed.py
File txzookeeper/tests/test_managed.py (right):

https://codereview.appspot.com/5976074/diff/1/txzookeeper/tests/test_managed....
txzookeeper/tests/test_managed.py:169: # It takes some time to propogate (1/3
session time as ping)
s/propogate/propagate/

[hazmat, 2012-04-04 18:09:00]

On 2012/04/04 13:23:29, fwereade wrote:
> This looks very nice indeed, but the vagueness of the "comes at a cost"
comment
> makes me wonder what I'm missing...

expanded greatly what those costs are.

> 
> https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py
> File txzookeeper/managed.py (right):
> 
> https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode66
> txzookeeper/managed.py:66: mgr  # keep the flakes happy
> Can't you just do:
> 
>   with self._ctx():
> 
> ?

much nicer, thanks.

> https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode70
> txzookeeper/managed.py:70: mgr  # keep the flakes happy
> Ditto
> 
done.
>
https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode122
> txzookeeper/managed.py:122: come at a cost though.
> Please expand ;).
> 

done.
>
https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode138
> txzookeeper/managed.py:138: """Called on intercept of session expiration to
> restablish the session.
> I'm pretty sure there are 2 es in re-establish, and that the hyphen may have
> fallen out of common use but remains IMO more readable. As you wish.
> 

proper speling established ;-)

> 
https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode140
> txzookeeper/managed.py:140: This will reconnect to zk, restabslish ephemerals,
> and trigger watches.
> I like the word "restabslish", but ditto.
> 
>
https://codereview.appspot.com/5976074/diff/1/txzookeeper/tests/test_managed.py
> File txzookeeper/tests/test_managed.py (right):
> 
>
https://codereview.appspot.com/5976074/diff/1/txzookeeper/tests/test_managed....
> txzookeeper/tests/test_managed.py:169: # It takes some time to propogate (1/3
> session time as ping)
> s/propogate/propagate/

done.

[hazmat, 2012-04-04 18:09:46]

Please take a look.

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py
File txzookeeper/managed.py (right):

https://codereview.appspot.com/5976074/diff/1/txzookeeper/managed.py#newcode66
txzookeeper/managed.py:66: mgr  # keep the flakes happy
On 2012/04/04 13:23:29, fwereade wrote:
> Can't you just do:
> 
>   with self._ctx():
> 
> ?

thanks, much nicer.

[jimbaker, 2012-04-07 00:08:39]

+1, LGTM, just some minors

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py
File txzookeeper/managed.py (right):

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newco...
txzookeeper/managed.py:153: def cb_restablish_session(self, e=None):
Presumably this should be cb_re_establish... but I don't see the difference here
with the easier synonym: restore. Good either way with me.

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newco...
txzookeeper/managed.py:162: # If its been explicitly closed, don't restablish.
it's... re-establish

(or are you saying something about being restful? ;) )

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newco...
txzookeeper/managed.py:166: # If its a stale handle, don't restablish
it's... re-establish

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newco...
txzookeeper/managed.py:174: # Its already been restablished, don't restablish.
It's... re-established... re-establish

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newco...
txzookeeper/managed.py:183: # Restablish
I'm not going to mark any more of these!

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/managed.py#newco...
txzookeeper/managed.py:189: log.error("error while restablish %r %s" % (e, e))
Another minor: probably want some consistency in beginning with uppercase. Not
certain why you use log.error here, but log.exception later for what appears to
be a similar case.

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/tests/test_manag...
File txzookeeper/tests/test_managed.py (right):

https://codereview.appspot.com/5976074/diff/4001/txzookeeper/tests/test_manag...
txzookeeper/tests/test_managed.py:110: """Reset fires a synthentic client event,
and clears watches.
no need for comma