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

On Thu, 08 Jul 2010 12:27:42 -0400, Kapil Thangavelu
<email address hidden> wrote:

>> The design of SerializedQueue looks pretty nice. Thank you!
>>
>> A few additional (last?) comments below:
>>
>> [20]
>>
>> In SerializedQueue:
>>
>> if name.endswith(suffix):
>> children[:] = []
>>
>> Putting some debugging information, I see that this logic is actually
>> used,
>> but I'm a bit confused regarding when it's necessary. The tests which
>> reach
>> this logic also make me a bit worried:
>> test_staged_multiproducer_multiconsumer, which theoretically should be
>> a case
>> which should never see items being processed due to the lock.
>
> the lock is only held when fetching an item. ie. the lock guards access
> to the queue items. However the consumer might still be processing the
> item, and until it does the processing node isn't released. it might be
> a better semantic (less contention) for serialization if the lock was
> held for the duration of the item processing. i'll make that change.
>

This is implemented now, the serialized queue works much smoother now with
less contention.
I've removed the children[:] filtering logic that was in use previously.

>
>> [21]
>>
>> Something that occurred to me while I was reading the code is that the
>> line:
>>
>> request.processing_children = False
>>
>> May still not get executed on certain exists from the logic in the item
>> processing. Should we guarantee that it will necessarily be reset on
>> any
>> exit, so that we never get an unusable object (with an inconsistent
>> state)?
>>
>
> is there a scenario where this would be the case? afaics all the failure
> mode exists are covered by error handlers that will set this flag to
> false.
>
after our discussion on irc, i took a look at putting the
request.processing_children into an errback/callback handler, but its not
clear that the same semantic would be achieved, Because one of the
scenarios is that a consumer is waiting on a producer to put items in the
queue, request.processing_children should be false here, but the no
callbacks would have been fired. ie.. a consumer attempts to fetch an item
 from a queue, there are some items in the queue, and the consumer is
processing children, but is competing with other consumers, if the queue
empties before an item is fetched, the consumer should wait on the watch
with processing_children = False with a callback/errback on the get the
flag wouldn't be set appropriately.

cheers,

Kapil