Code review comment for lp:~hazmat/txzookeeper/managed-watch-and-ephemeral

Revision history for this message
William Reade (fwereade) wrote :

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.py#newcode169
txzookeeper/tests/test_managed.py:169: # It takes some time to propogate
(1/3 session time as ping)
s/propogate/propagate/

https://codereview.appspot.com/5976074/

« Back to merge proposal