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

This looks very good. A few comments, and +1 with them considered.

[1]

+ zookeeper.NOTWATCHING_EVENT: "notwatching",
+ zookeeper.SESSION_EVENT: "session",
+ zookeeper.CREATED_EVENT: 'created',

Quotes are inconsistent here.

Also, "not-watching" would be a bit more friendly to the eye.

[2]

+def debug_symbols(sequence):

Left over?

[3]

error = error_class(error_msg)
+
+ if ConnectionException.is_connection_exception(error):

This idiom feels a little weird. A Foo is necessarily a Foo,
so we never have Foo.is_foo. Unless you have some background
for the choice that I miss, I suggest just moving that to a
"is_connection_exception()" top-level function.

[4]

+ # The result of the connection error handler is returned
+ # to the api.

That's a nice design, despite our conversations around the problem of
connection level erroring.

I'm a bit concerned about the session events, though. A session-termination
event may be the only event the deferred ever sees, so if we divert it to
a separate callback, the logic pending on the deferred will be dead forever.

I'm not sure about what we should do here. Have you thought about this?

If the answer is to just be so for now, we should at least add a note to
the proper location in the code mentioning that these deferreds are dead.

[5]

+ d = defer.maybeDeferred(
+ self._connection_error_callback,
+ self, error)

For consistency, either both callbacks should take "self", or neither of them
should.

[6]

+ # XXX/Debug
+ #print
+ #print "Session/Conn Event", ClientEvent(type, state, path)
+

Some left overs.

[7]

+ self._check_result(result, d)
+ if not d.called:
+ d.callback(True)

This is checking if the deferred is being called synchronously, which seems
dangerous as a pattern. Theorethically, the deferred may not have fired by
the time the function returns.

I suggest using the interface defined (considering the suggestions from
the pre-requisite branch):

  if not self._failed(result, d):
      d.callback(True)
  return d