qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kenneth Giusti" <kgiu...@apache.org>
Subject Re: Review Request: Prototype of infrastructure changes needed to support message groups [PRELIMINARY]
Date Mon, 08 Aug 2011 21:21:03 GMT


> On 2011-08-08 10:11:17, Gordon Sim wrote:
> > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h, line 132
> > <https://reviews.apache.org/r/1312/diff/1/?file=30947#file30947line132>
> >
> >     I'm not keen on the terminology here. For me selector implies something subtly
different from the role this object is serving (at least from the role I *think* it is serving).
> >     
> >     I'd prefer something like 'allocator'.

Me too - renamed to allocator.


> On 2011-08-08 10:11:17, Gordon Sim wrote:
> > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h, line 148
> > <https://reviews.apache.org/r/1312/diff/1/?file=30947#file30947line148>
> >
> >     Again, I'm not too keen on the term 'consumed' in this context. Though I can
see how it is justified, it is potentially confusing in my view (could imply the actual dequeue
of a message).
> >     
> >     I'd prefer 'acquired', 'allocated' or even just 'locked' as they are all less
ambiguous on the state in question.

Agreed - renamed to acquired.


> On 2011-08-08 10:11:17, Gordon Sim wrote:
> > /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp, line 158
> > <https://reviews.apache.org/r/1312/diff/1/?file=30945#file30945line158>
> >
> >     Not too keen on this lookup. Can it be avoided?
> >     
> >     E.g. can we modify the Queue::acquire() to simply take the consumer name as
the second parameter? (That is for the present at least all that is required)
> >     
> >     Alternatively the DeliveryRecord could be modified to hold a pointer to the
Consumer rather than simply the tag.

Modified Queue::acquire() to take the name instead - should be ok.   I originally had added
a pointer to the Consumer into the DeliveryRecord, but that raised issues with cluster replication
(Consumer not present when replicating DeliveryRecord).  May have to revisit this if it turns
out we need the pointer.


> On 2011-08-08 10:11:17, Gordon Sim wrote:
> > /branches/qpid-3346/qpid/cpp/src/qpid/broker/LegacyLVQ.cpp, line 39
> > <https://reviews.apache.org/r/1312/diff/1/?file=30946#file30946line39>
> >
> >     The purpose of the check is to ensure that an acquire attempt for a message
that has since been replaced, does not acquire the message that has replaced it instead.
> >     
> >     I believe it is still necessary, though I concede the form is ugly and unintuitive.

Agreed that it is necessary for LVQ, but the abstract api for 'Messages::remove' can't explicitly
enforce that the caller supply the actual target msg, just the position.  Given that, it's
pretty easy for the caller to get this wrong without discovering it (which actually happened
to me - thank goodness for unit tests :).

And the "move message/purge message" management operations probably could use a remove method
that doesn't necessitate a preceding message find().  

I'm thinking of two different message-removal use-cases:

bool Messages::remove( position, QueuedMessage *result) - remove msg at position, return true
and set result if found
bool Messages::remove( const QueuedMessage& target ) - remove target msg, return true
if msg was found.   This variant could be used by 'acquire' or other paths that have already
retrieved the QueuedMessage.

Although, defining a iterator for the Messages class probably renders this whole issue moot.


- Kenneth


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1312/#review1317
-----------------------------------------------------------


On 2011-08-05 20:46:15, Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1312/
> -----------------------------------------------------------
> 
> (Updated 2011-08-05 20:46:15)
> 
> 
> Review request for qpid.
> 
> 
> Summary
> -------
> 
> Some initial refactoring of Queue/Consumer interface to allow for message grouping support.
 Very preliminary.
> 
> 
> This addresses bug qpid-3346.
>     https://issues.apache.org/jira/browse/qpid-3346
> 
> 
> Diffs
> -----
> 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/Consumer.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/LegacyLVQ.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueEvents.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueObserver.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueuePolicy.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/SemanticState.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/SemanticState.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1144324 
>   /branches/qpid-3346/qpid/cpp/src/qpid/cluster/Connection.cpp 1144324 
>   /branches/qpid-3346/qpid/cpp/src/tests/QueueTest.cpp 1144324 
> 
> Diff: https://reviews.apache.org/r/1312/diff
> 
> 
> Testing
> -------
> 
> minimal - passes unit tests on linux.
> 
> 
> Thanks,
> 
> Kenneth
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message