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

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Just for the record, as we discussed over the phone:

[1]

The notification abstractions hide a bit the intention behind the way that the ZooKeeper API is designed: watchers are set up on data retrieval because it only makes sense to be told about changes once you know the existing value, and once changes happen the client must ask again about the value and reset the notification if it's still interested in knowing about changes.

As a consequence of this model, fast subsequent changes won't create a swarm effect, because all the clients are told about is that the value isn't the same they originally knew about anymore, but more than one change may have happened, and that's still alright.

With that in mind, I suggest having something like this, in addition to get_data() and get_children():

    def get_data_and_watch(self):
        (...)
        return data_deferred, watch_deferred

    def get_children_and_watch(self):
        (...)
        return children_deferred, watch_deferred

This uses twisted deferreds as well.

[2]

This isn't about a specific action required.. just a feeling I had.

I understand the intention behind the internal keeping of the node versioning, and I think it's a good model to start with. It's just not entirely clear to me how we'll want these things to behave in practice, but we'll certainly figure it out.

review: Needs Fixing

« Back to merge proposal