Code review comment for lp:~juju/txzookeeper/basic-node-api

[3]

Thanks for the watch implementation. It looks sensible.

As we discussed, one thing I wonder is if we should change the watch parameter into an Event object with "node" (the object, rather than the path), "kind", and "status". Might potentially make it more comfortable to consume the event.

[4]

+ def __cmp__(self, other):
+ return cmp(self.path, other.path)

This should be turned into __eq__ I believe, otherwise we'll get weird calls like node1 < node2 working in an arbitrary way, when they should error out.

[5]

+ if self._node_exists:
+ # if the node is deleted while we have this cached state we should
+ # covert the set to a create. XXX ??? Should we

On a second look, this starts to smell a bit strange. If we look at the algorithms created around ZooKeeper, they're generally very sensitive about when things are created, changed, deleted, etc. Having our Node API doing things behind the programmer's back feels like a hole we shouldn't get into. It reminds me of the sqlite Python wrapper, which does a lot of "smart" magic with the transaction, and as a side effect turns transactions into something unreliable (which is very ironic).

I understand and agree that having a create() method in a Node object which already exists may feel a bit weird from an OO perspective, but I really think we should go towards this way if we want to make the logic being created on top of this really easy to follow.

If we don't do this, we'll always to have to keep in mind "Ohh, this method call actually *creates* the node too!" when thinking of higher-level algorithms on top of ZooKeeper, which feels undesirable.

What do you think?