Code review comment for lp:~hazmat/txzookeeper/session-event-handling

+1, we certainly need this branch, and it looks quite solid.

In addition to Gustavo's comments:

[1]

Some minor grammar suggestions:

+ specified in comma separated fashion, if they are,

specified in comma separated fashion. If they are,

+ # If its a session event pass it to the session callback, else

If it's a session event, ...

+ nodes and watches)/ The connection's client id, is also useful
+ when introspecting the server logs for specific client
+ activity.

nodes and watches. The connection's client id is also...

+ Its used for all connection level events and session events.

It's used...

+ # Send session events to the callback, this in addition to any
+ # duplicate session events that will be sent for extant watches.

callback, in addition

+ """Set a callback to recieve session events.

receive

+ twisted integration using deferreds is not capable of recieving

receiving

+ to be invoked with them instead. The callback recieves a single

receives

+ call is made, by setting a connection level error handler,
+ applications can centralize their handling of connection loss,

Something like

By setting...

+ This is a global setting, across connections.

setting across all connections

+ A client session can be reattached to in a separate connection,
+ if the a session is expired, using the zookeeper connection will
+ raise a SessionExpiredException.

+ # On occassion we get a session expired event while connecting.

occasion

+ # Closing one connection, will close the session

Closing one connection will close the session.

+ A client connected to multiple servers, will transparently
+ migrate amongst them, as individual servers can no longer be
+ reached. A client's session will be maintined.

servers will transparently...

...will be maintained.

+ # get a zookeeper connectionloss exception on occassion.

occasion

+ We can specify a callback for connection errors, that
+ can perform recovery for a disconnected client, restablishing

This seems incomplete; also there should not be a comma before "that"

+ """On connection failure, An application can choose to use a
+ new connection with to reconnect to a different member of the
+ zookeeper cluster, reacquiring the extant session.

an application... with which to reconnect...

Maybe this concludes with "This enables reacquiring the existing
session." (Extant doesn't seem like the right word choice here.)

+ A large obvious caveat to using a new client instance rather
+ than reconnecting the existing client, is that even though the

instance, rather than...

+ # Ephemeral is destroyed when the session closed.

the session is closed.

[2]

+ http://bit.ly/mQrOMY
+ http://bit.ly/irKpfn

Why not just expand these links out? Alternatively use a readable URL
(the underlying nabble URL is modestly readable, I don't know how
permalink either one is, however).

[3]

+ assert callable(callback)

Shouldn't this be throwing a TypeError instead? Also, there is no
correponding assertion (or verification) on
set_connection_error_callback. Lastly, there is no test on this path.

[4]

+ def xtest_session_expired_event(self):

Do you want to include this test?