Code review comment for lp:~hazmat/txzookeeper/distributed-queue

On Fri, 28 May 2010 16:30:42 -0400, Gustavo Niemeyer
<email address hidden> wrote:

> [1]
>
> + self.path = path
> + self.client = client
> + self.persistent = persistent
>
> I tend to prefer keeping things private, unless they are actually
> required by external clients, or would be obviously important to know
> about. This way it's easy to tell what's the "trusted" API clients are
> supposed to rely on, and also makes it more comfortable when changing
> the interface (anything private can be removed/renamed/replaced).
>
> How do you feel about this in general?

In general i'm fine with it. i have mixed feeling about private is spelled
in some cases, i've dealt with libraries that take private (double under
style) to levels that made clean reuse or debugging difficult. In this
particular case, i'm fine with client being private (single underscore),
and path and persistent being read only properties and have made those
changes. In general its okay as long its not a double underscore and there
isn't a legitimate reason for the its use by consumer, if the latter i'd
tend to prefer at least read only property access if direct attribute
access would be dangerous.

>
>
> [2]
>
> + self, path, client, acl=[ZOO_OPEN_ACL_UNSAFE],
> persistent=False):
>
> Also a pretty general point which I'm making mostly to synchronize our
> thinking, rather than as a *required* change here:
>
> I tend to prefer using the style of acl=None in the initialization of
> default parameters, and then process it internally in the constructor
> (if acl is None, acl = ... or similar).
>
> While the result is obviously the same here, the main distinction is
> that it enables code using the function or class to say "I don't have
> anything special in this parameter.. just do your default.", which gets
> pretty tricky when the default value is in the keyword argument
> constructor itself.
>

i'm fine with that, to me its a minor tradeoff to quick reading the
function signature, but then again there's a whole class of bugs around
mutable default args. changed.

>
> [3]
>
> + return self._get(wait=True)
> +
> + get_wait = get
>
> Do we need all these alternatives? Part of the beauty of Twisted is
> that we don't really have to care about what "waiting" means.

right, originally i was trying to match signature provided by the queue
implementation. i think this alias is an artifact of when i changing the
defaults behavior of get (used to raise empty error). its removed now.

>
> I suggest we have a single interface for getting, and it will always
> return a deferred which will fire once an item is available, no matter
> what. This will also simplify a bit the logic elsewhere (in _refill,
> _get_item, etc).
>

hmmm.. i like having the option of both apis and the synchronicity to the
Queue.Queue api. alternatively perhaps a timeout option on the get api
would also suffice (also in Queue.Queue api). the logic simplification
 from the method removal is pretty minor, maybe four lines of code total
in the methods mentioned.

>
>
> [6]
>
> + Fetch the node data in the queue directory for the given node
> name. If
> + wait is
> (...)
> + # tests. Instead we process our entire our node cache
> before
> + # proceeding.
>
> Couple of comment details: "is ..." and "our entire our".
>

fixed and reworded.

> [7]
>
> + Refetch the queue children, setting a watch as needed, and
> invalidating
> + any previous children entries queue.
>
> It would be nice to have a higher level description of what the
> algorithm is actually doing. E.g. what is the children entries queue
> about, what happens when it's empty, or when two different consumers
> have a partially overlapping queue, how are items consumed, etc.
>

noted

>
> [8]
>
> In on_queue_items_changed():
>
> + self._cached_entries = []
> + d = self._refill(wait=wait)
>
> Why is the cache being reset right after we're told changes have
> happened? Shouldn't this happen once we actually get the new list of
> children?

its being reset to prevent processing of possibly invalid cache entries,
when we know we have an out of date list. The line can be removed without
affecting the algorithm, its just a tradeoff to reduce a few redundant
roundtrip item fetches in high concurrency (test? ;-) scenarios. i've gone
ahead and removed it.
>
> [9]
>
> + d = self._refill(wait=wait)
> (...)
> + d = self.client.get_children(
> + self.path, on_queue_items_changed)
>
> It'd be good to have more descriptive names for these variables, since
> one of them is shadowing the other. Note, for instance:
>
> + d.addCallback(notify_waiting)
> +
> + d = self.client.get_children(
>
> The "d" above isn't the "d" below, even though they share a scope and
> are so close.
>
>
> [10]
>
> + if isinstance(failure.value, zookeeper.NoNodeException):
>
> Failure has a trap() method which is handy for these cases.
>

nice, thanks.

> [11]
>
> This is a gut feeling, but it feels a bit like there are too many entry
> points into the same caching logic. There are four places testing for
> the emptiness of _cached_entries, and at least four other places
> modifying it for various reasons, in different functions. It'd be
> really good for us if we can streamline and simplify this into a single
> place popping items, and a single place refilling it.
>
> If you don't feel like working on this, I can have a look at refactoring
> this somehow to see if that's feasible, once these branches go in and we
> have a stable base again.
>

i took a stab at reducing cache entries modifications points, by
restarting the get unconditionally in _get_item instead of processing
cache entries, but it lead to consumer underflows in the staged producer
/consumer test. i'm going to leave off on this for now.