----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1980/#review1995 -----------------------------------------------------------
It would be good to verify latency as well. qpid-cpp-benchmark will measure both together. what is the purpose of the new queue-query management method? e-move to message groups? Some useful code documentation added, and code simplified in several places good job. Looks good, a few comments & questions. /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.cpp <https://reviews.apache.org/r/1980/#comment4496> Shouldn't this return false if the message has already been acquired by some other consumer? /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.h <https://reviews.apache.org/r/1980/#comment4497> This looks a lot like the new cluster interface. There's definitely room for convergence of the "broker activity watchers" - but that's another JIRA. /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.cpp <https://reviews.apache.org/r/1980/#comment4498> The style guide for constants is CAPITAL_UNDERSCORES. /trunk/qpid/cpp/src/qpid/broker/MessageGroupManager.cpp <https://reviews.apache.org/r/1980/#comment4499> I don't like the #ifdef with different code on each branch. You could say: assert(freegroups.find(qm.position) == freegroups.end()); freeGroups[qm.position] = &state; /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/1980/#comment4501> Are you sure it is thread safe to split this lock scope apart? /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/1980/#comment4505> Is there a comment somewhere outlining the locking rules? E.g. must call acquired inside the messasgeLock, may call dequeue outside. While it's fresh in your mind ;) /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/1980/#comment4507> This pattern of loop shows up a lot. Would be good to encapsulate it in some short-hand but I'm not sure how. /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/1980/#comment4502> Shouldn't need a cast here. /trunk/qpid/cpp/src/qpid/broker/QueueObserver.h <https://reviews.apache.org/r/1980/#comment4504> Suggest not introducing new terms here but using terms from the code, so the states would be: "enqueued", "acquired", "dequeued" These match the names of the corresponding events below which is a good thing. /trunk/qpid/cpp/src/qpid/broker/QueueObserver.h <https://reviews.apache.org/r/1980/#comment4503> Should be "acquired" to be consistent with the naming in the code. - Alan On 2011-09-20 15:14:26, Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1980/ > ----------------------------------------------------------- > > (Updated 2011-09-20 15:14:26) > > > 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/MessageAllocator.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/MessageAllocator.cpp 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 > >
