qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request: QPID-3079: allow asynchronous completion of Message.Accept command (note: requires store interface changes).
Date Fri, 10 Jun 2011 12:56:08 GMT


> On 2011-06-10 00:22:59, Kenneth Giusti wrote:
> > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp, line 685
> > <https://reviews.apache.org/r/860/diff/1/?file=20822#file20822line685>
> >
> >     The P.M. _could_ be being dequeued simultaneously from multiple queues.  Won't
that just move the map from the queue to the P.M.?

I'm suggesting "pmsg->setCompletionCallback(callback)". No map. setCompletionCallback()
may  be called concurrently in multiple deques but that does't require a map, just a lock.
I think the completion context belongs on the message.


> On 2011-06-10 00:22:59, Kenneth Giusti wrote:
> > /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp, line 1229
> > <https://reviews.apache.org/r/860/diff/1/?file=20822#file20822line1229>
> >
> >     Perhaps - I still think we'd need to track multiple outstanding callbacks for
a single message instance.

Why? All  you need to know is when they all complete. A simple atomic counter will do the
job. I think we should be aiming to avoid multiple heap allocations per message wherever we
can. Ideally I'd like to see the completion context boiled down to a few counters and allocated
in-line as part of the Message. The interfaces are good, but the implementation does a lot
of dynamic allocation. I suspect that will hurt performance.


> On 2011-06-10 00:22:59, Kenneth Giusti wrote:
> > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 799
> > <https://reviews.apache.org/r/860/diff/1/?file=20825#file20825line799>
> >
> >     Actually, it's very likely that all the messages that are in a Message.accept
sequence are on the same queue.  I noticed that during debug - a flush of a single message.accept
command resulted in multiple flush calls to the same queue.  According to Kim, multiple flushes
to the same queue are ignored, so I could drop this, but that may not be true for store in
general.

Why do we need a flush() on the AsyncMessageAcceptCommand?


> On 2011-06-10 00:22:59, Kenneth Giusti wrote:
> > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 862
> > <https://reviews.apache.org/r/860/diff/1/?file=20825#file20825line862>
> >
> >     There may be multiple pending dequeues for a given message, so we would need
some sort of container anyway.

I'd prefer a counter to a container.


> On 2011-06-10 00:22:59, Kenneth Giusti wrote:
> > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 876
> > <https://reviews.apache.org/r/860/diff/1/?file=20825#file20825line876>
> >
> >     I'm not really happy with this approach, but I can't think of a way to track
N outstanding dequeue operations without using some dynamic container.
> >     
> >     Each callback is actually a "functor" object that contains state needed to map
the dequeue back to the pending Message.Accept.  This state impl is hidden from the Message
object - I'd hate to have to expose command-specific state into the messages (but I'm probably
not thinking abstractly enough at this point :P )

As long as the command-specific state is encapsulated in its own class, I think it's fine
to attach it to the message. In fact I think we need a general solution for "feature x has
per-message state"  that avoids keeping a map of all the messages. Not for this patch though
:)


> On 2011-06-10 00:22:59, Kenneth Giusti wrote:
> > /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp, line 930
> > <https://reviews.apache.org/r/860/diff/1/?file=20825#file20825line930>
> >
> >     The behaviour of the unacked list should be the same as it is now.  I'm more
worried about the completion of the Message.Accept cmd, which will happen after the store
has completed dequeue.  That will be a visible change in behaviour and I don't think clustering
will like it.

Completion of the accept is sent in a control, not a command. So it doesn't disturb the command-ids
of outgoing transfers. Need to review  if there's any potential for trouble with the rest
of the Session state. Gordon?


- Alan


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


On 2011-06-08 20:31:05, Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/860/
> -----------------------------------------------------------
> 
> (Updated 2011-06-08 20:31:05)
> 
> 
> Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
> 
> 
> Summary
> -------
> 
> Modifies the broker's handling of Message.Accept to hold off the completion of the command
until all messages related to the accept have completed dequeue.  This particularly applies
to persistent messages, as the store::dequeue() operation must complete before the message
is considered fully dequeued.
> 
> Note this bugfix requires some changes to the broker's store module interface:  previously,
the store only identified the message when a dequeue was completed.  This is not enough information
- the queue from which is was removed must also be identified (the message may be in the process
of being dequeued on several queues at once).
> 
> 
> This addresses bug qpid-3079.
>     https://issues.apache.org/jira/browse/qpid-3079
> 
> 
> Diffs
> -----
> 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/AsyncCompletion.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStore.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableQueue.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoverableQueue.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionContext.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/tests/AsyncCompletion.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/tests/QueueTest.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/tests/TestMessageStore.h 1124895 
> 
> Diff: https://reviews.apache.org/r/860/diff
> 
> 
> Testing
> -------
> 
> broker unit tests, store unit tests (modified jboss store).   Still needs to be vetted
on non-linux, and have latest trunk merged in.
> 
> 
> Thanks,
> 
> Kenneth
> 
>


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