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: QPID-3346: message groups (multi-consumer per group)
Date Wed, 28 Sep 2011 13:59:58 GMT


> On 2011-09-28 12:48:39, Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.h, line 91
> > <https://reviews.apache.org/r/1980/diff/3/?file=46025#file46025line91>
> >
> >     Don't put different logic in debug/release builds. Asserts are fine but this
is calling a different function. If there's a bug in the NDEBUG function then developer testing
won't uncover it. (even with asserts you need to be careful that they don't have side-effects,
we've been bitten by that before)

Ouch - good catch, thought I removed those NDEBUG paths - I'll pull it.


> On 2011-09-28 12:48:39, Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.h, line 59
> > <https://reviews.apache.org/r/1980/diff/3/?file=46024#file46024line59>
> >
> >     Suggest calling this acquire() since that's what is used elsewhere to refer
to assigning ownership.

I used that name at first, but the Queue code also has "acquire" methods, and I thought it
was confusing to see the allocator acquire the message, and the Queue also acquire the message.
 See below for more thoughts on the relationships of these objects...


> On 2011-09-28 12:48:39, Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.h, line 37
> > <https://reviews.apache.org/r/1980/diff/3/?file=46024#file46024line37>
> >
> >     How about QueueOrder? It defines the order that consumers see messages. Or perhaps
something like MessageChooser

"A Rose by Any Other Name" :)

I had originally thought of calling it MessageSelector...   - see below for my thoughts on
the purpose of this class vs Messages class, and why I've separated the two.


> On 2011-09-28 12:48:39, Alan Conway wrote:
> > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp, line 406
> > <https://reviews.apache.org/r/1980/diff/3/?file=46029#file46029line406>
> >
> >     Shouldn't MessageAllocator replace the Messages interface? They are both supposed
to store messages and determine the order in which they are acquired. 
> >     Why are we sometimes choosing the next message via the MessageAllocator and
other times via the Messages? Also aren't we doubling the storage?

Let me take a step back and explain my thoughts regarding these different classes:

Currently, the Messages abstraction is used as the interface for the broker::Queue's in-memory
storage for queued messages.  It provides two services to the queue:  1) storage of the queued
messages, and 2) Ordering of those stored messages (IMPORTANT).   Different implementations
of Queues (Priority, FIFO, LVQ, etc) can define their method of ordering which is hidden from
the Queue's implementation.

Personally, I'd love to rename Messages to "MessageContainer", as I think that's more descriptive
of its purpose.  But I digress....

There are places in the code where the Queue merely needs to get the head "available" (non-acquired)
message with out dispatching it to a client.  For these cases, choosing a message via Messages
is optimal, and there is no need for MessageAllocator.

However, with the addition of message groups, the -ordering- of available messages is not
enough to determine which of the available messages can be dispatched to a consumer.  Now,
we have to also consider the particular consumer that is requesting a message in order to
select the correct message from those that are available.

I don't want to overload the Messages class with yet more state (like tracking which Consumer
has acquired what message) - I think it is best to keep Messages simple.

That's where MessageAllocator (aka MessageSelector, MessageChooser, MessageFinagler) comes
in: its purpose is to determine which message - from the sequence of available messages as
determined by the Messages object - is the proper message to dispatch to a Consumer.  Unlike
Messages, it does not store the message, nor does it know (or care) about message ordering
(again, the job of Messages).   

** So these two objects allow us to separate the ordering and storage of a queued message
from the intelligence that picks which among the ordered available messages is the "best"
for a given consumer.  They become orthogonal, and this allows various combinations of features
to be supported. **

For example, with the non-grouped message case the MessageAllocator simply picks the head
available message (see FifoAllocator impl).    But for the message group implementation of
the MessageAllocator (see MessageGroupManager impl), it not only uses the groupId of the available
messages, but also considers the identify of the Consumer, the Consumer's position in the
queue, and the availability of the group itself.   All hidden from the Queue implementation.
 


- Kenneth


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


On 2011-09-27 20:26:22, Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1980/
> -----------------------------------------------------------
> 
> (Updated 2011-09-27 20:26:22)
> 
> 
> Review request for qpid, Alan Conway and Gordon Sim.
> 
> 
> Summary
> -------
> 
> This implements QPID-3346.  I'd like to get this onto trunk.
> 
> This patch does not include optimizations I have made - I'll be reviewing & submitting
these separately as they involve more intrusive changes to the Queue API. Ignore for now the
TODO comments in the MessageGroups source files.
> 
> Performance testing suggests that this patch has negligible effect on legacy traffic
performance (non-grouped traffic):
> 
> current trunk:
> [kgiusti@mrg42 tests]$ ./qpid-perftest -b 20.0.10.43 --iterations 5 --count 500000 --npubs=1
--nsubs=1 --qt=1 --summary
> 58096.5	56556.7	113124	110.473
> 62659.7	62195.4	124381	121.466
> 54278.2	54136.2	108265	105.728
> 55506.3	55501	110505	107.915
> 62176.8	58019.1	115500	112.793
> Averages: 
> 58543.5	57281.7	114355	111.675
> 
> + this patch:
> [kgiusti@mrg42 tests]$ ./qpid-perftest -b 20.0.10.43 --iterations 5 --count 500000 --npubs=1
--nsubs=1 --qt=1 --summary
> 58346.1	55008.2	110510	107.92
> 59309.8	58747.5	117482	114.729
> 61323.3	58222.1	116436	113.707
> 57603	57600.4	115193	112.493
> 56091.8	56086.4	111667	109.05
> Averages: 
> 58534.8	57132.9	114258	111.58
> 
> 
> This addresses bug qpid-3346.
>     https://issues.apache.org/jira/browse/qpid-3346
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/CMakeLists.txt 1172628 
>   /trunk/qpid/cpp/src/Makefile.am 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/Consumer.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/FifoAllocator.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/broker/FifoAllocator.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.h PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/qpid/broker/Messages.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/Queue.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/QueueEvents.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/QueueObserver.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/QueuePolicy.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1172628 
>   /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1172628 
>   /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1172628 
>   /trunk/qpid/cpp/src/tests/CMakeLists.txt 1172628 
>   /trunk/qpid/cpp/src/tests/Makefile.am 1172628 
>   /trunk/qpid/cpp/src/tests/QueueTest.cpp 1172628 
>   /trunk/qpid/cpp/src/tests/cluster_tests.py 1172628 
>   /trunk/qpid/cpp/src/tests/msg_group_test.cpp PRE-CREATION 
>   /trunk/qpid/cpp/src/tests/qpid-send.cpp 1172628 
>   /trunk/qpid/cpp/src/tests/run_msg_group_tests PRE-CREATION 
>   /trunk/qpid/cpp/src/tests/run_msg_group_tests_soak PRE-CREATION 
>   /trunk/qpid/java/broker/src/main/java/org/apache/qpid/qmf/QMFService.java 1172628 
>   /trunk/qpid/specs/management-schema.xml 1172628 
>   /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/management.py 1172628 
> 
> Diff: https://reviews.apache.org/r/1980/diff
> 
> 
> Testing
> -------
> 
> new unit tests & tools added.   Built cpp & java on Windows & various linux
distros.
> 
> 
> Thanks,
> 
> Kenneth
> 
>


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