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

On Fri, 11 Jun 2010 10:57:30 -0400, Gustavo Niemeyer
<email address hidden> wrote:

> Review: Needs Fixing
> Thanks Kapil. Queue looks great. Some items about the new stuff:
>
> [15]
>
> + and processed in the order they where placed in the queue. (TODO) An
> + implementation with less contention between consumers might instead
> utilize
> + a reliable queue with a lock.
>
> Even without the lock, a better implementation would wait for the
> removal of the specific -processing file which is holding further action
> back, rather than any change in the queue. E.g. if the item itself is
> removed, but not the -processing control file, it will fire off, and
> fail again. Also (and most importantly), any new items added to the
> queue will also fire the watch and cause the consumer to refetch the
> full child list only to find out that it's still being processed by
> someone else, and it can do nothing about the new item appended at the
> back of the queue.

That would definitely be a better implementation than what was there
previously wrt to contention. However it would effectively be
reimplementing parts of the lock logic (ie. things like check exists
return value on the processing node is equivalent to checking exists on
the previous lock candidate node). I went ahead and reimplemented the
serialized queue using a subclass of reliable queue that utilizes an
exclusive lock to serialize access.

>
> [16]
>
> +class SerializedQueueTests(QueueTests):
> +
> + queue_factory = SerializedQueue
>
> Don't we need something here? :-)

yeah.. i'm not sure what offhand. The default implementation of all the
queues are ordered. Perhaps a multiple clients, one which gets an item and
then sleeps a close.

>
> [17]
>
> Queue should probably use QueueItems too (without delete). That said,
> since it's just a reference implementation and we'll likely need the
> reliable version, feel free to ignore this.
>

cool, i'm just going to leave it as is. I tried to be clear in the doc
string that its mostly meant as example implementation of the common
zookeeper recipe.

> [18]
>
> It feels like there's potential for more reuse in the _get_item
> implementation, but I don't think we should wait this for merging. It's
> just a good refactoring for a boring moment.
>

definitely, the closures are convient, but make things a bit more
difficult for clean reuse. i took a stab at this yesterday, but shelved it
for now. Effectively we stuff the extra args for callbacks and errbacks
instead of relying on the closure.