[
https://issues.apache.org/jira/browse/QPID-3346?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13116497#comment-13116497
]
[email protected] commented on QPID-3346:
-----------------------------------------------------
bq. On 2011-09-28 12:48:39, Alan Conway wrote:
bq. > /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.h, line 91
bq. > <https://reviews.apache.org/r/1980/diff/3/?file=46025#file46025line91>
bq. >
bq. > 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.
bq. On 2011-09-28 12:48:39, Alan Conway wrote:
bq. > /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.h, line 59
bq. > <https://reviews.apache.org/r/1980/diff/3/?file=46024#file46024line59>
bq. >
bq. > 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...
bq. On 2011-09-28 12:48:39, Alan Conway wrote:
bq. > /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.h, line 37
bq. > <https://reviews.apache.org/r/1980/diff/3/?file=46024#file46024line37>
bq. >
bq. > 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.
bq. On 2011-09-28 12:48:39, Alan Conway wrote:
bq. > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp, line 406
bq. > <https://reviews.apache.org/r/1980/diff/3/?file=46029#file46029line406>
bq. >
bq. > Shouldn't MessageAllocator replace the Messages interface? They are
both supposed to store messages and determine the order in which they are
acquired.
bq. > 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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1980/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-09-27 20:26:22)
bq.
bq.
bq. Review request for qpid, Alan Conway and Gordon Sim.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. This implements QPID-3346. I'd like to get this onto trunk.
bq.
bq. 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.
bq.
bq. Performance testing suggests that this patch has negligible effect on
legacy traffic performance (non-grouped traffic):
bq.
bq. current trunk:
bq. [kgiusti@mrg42 tests]$ ./qpid-perftest -b 20.0.10.43 --iterations 5
--count 500000 --npubs=1 --nsubs=1 --qt=1 --summary
bq. 58096.5 56556.7 113124 110.473
bq. 62659.7 62195.4 124381 121.466
bq. 54278.2 54136.2 108265 105.728
bq. 55506.3 55501 110505 107.915
bq. 62176.8 58019.1 115500 112.793
bq. Averages:
bq. 58543.5 57281.7 114355 111.675
bq.
bq. + this patch:
bq. [kgiusti@mrg42 tests]$ ./qpid-perftest -b 20.0.10.43 --iterations 5
--count 500000 --npubs=1 --nsubs=1 --qt=1 --summary
bq. 58346.1 55008.2 110510 107.92
bq. 59309.8 58747.5 117482 114.729
bq. 61323.3 58222.1 116436 113.707
bq. 57603 57600.4 115193 112.493
bq. 56091.8 56086.4 111667 109.05
bq. Averages:
bq. 58534.8 57132.9 114258 111.58
bq.
bq.
bq. This addresses bug qpid-3346.
bq. https://issues.apache.org/jira/browse/qpid-3346
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /trunk/qpid/cpp/src/CMakeLists.txt 1172628
bq. /trunk/qpid/cpp/src/Makefile.am 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/Broker.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/Consumer.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/FifoAllocator.h PRE-CREATION
bq. /trunk/qpid/cpp/src/qpid/broker/FifoAllocator.cpp PRE-CREATION
bq. /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.h PRE-CREATION
bq. /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.h PRE-CREATION
bq. /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.cpp PRE-CREATION
bq. /trunk/qpid/cpp/src/qpid/broker/Messages.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/Queue.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/QueueEvents.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/QueueObserver.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/QueuePolicy.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/SemanticState.h 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1172628
bq. /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1172628
bq. /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1172628
bq. /trunk/qpid/cpp/src/tests/CMakeLists.txt 1172628
bq. /trunk/qpid/cpp/src/tests/Makefile.am 1172628
bq. /trunk/qpid/cpp/src/tests/QueueTest.cpp 1172628
bq. /trunk/qpid/cpp/src/tests/cluster_tests.py 1172628
bq. /trunk/qpid/cpp/src/tests/msg_group_test.cpp PRE-CREATION
bq. /trunk/qpid/cpp/src/tests/qpid-send.cpp 1172628
bq. /trunk/qpid/cpp/src/tests/run_msg_group_tests PRE-CREATION
bq. /trunk/qpid/cpp/src/tests/run_msg_group_tests_soak PRE-CREATION
bq.
/trunk/qpid/java/broker/src/main/java/org/apache/qpid/qmf/QMFService.java
1172628
bq. /trunk/qpid/specs/management-schema.xml 1172628
bq. /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/management.py 1172628
bq.
bq. Diff: https://reviews.apache.org/r/1980/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. new unit tests & tools added. Built cpp & java on Windows & various
linux distros.
bq.
bq.
bq. Thanks,
bq.
bq. Kenneth
bq.
bq.
> Support message grouping with stricted sequence consumption across multiple
> consumers.
> --------------------------------------------------------------------------------------
>
> Key: QPID-3346
> URL: https://issues.apache.org/jira/browse/QPID-3346
> Project: Qpid
> Issue Type: New Feature
> Components: C++ Broker
> Affects Versions: 0.12
> Reporter: Ken Giusti
> Assignee: Ken Giusti
> Fix For: 0.14
>
> Attachments: UI.txt, msg_groups_0.2.txt
>
>
> This feature is described in the attached QIP as Policy #2: Sequenced
> Consumers.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
