It seemed reasonable to me to keep it within the queue code, because browse-only-ness is a property of queues, and not of sessions.
By moving the enforcement of browse-only-ness out of the queue code, it seems to me that opens a possibility that some other pathway will someday be created -- not through the SessionAdapter code -- that will unknowingly violate the browse-only-ness of the Queue. Innit ? On Thu, 2012-09-06 at 16:57 +0000, Gordon Sim wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6943/ > > I'm not hugely keen on this approach, in particular the changes to > Queue::getNextMessage() and Queue::dequeue(). I don't understand why you want > to keep the changes within the queue. It seems cleaner for me to do this at > the earliest point possible, when actually handling the subscribe request. > E.g. in conjunction with your change to QueueSettings: > > Index: cpp/src/qpid/broker/SessionAdapter.cpp > =================================================================== > --- cpp/src/qpid/broker/SessionAdapter.cpp (revision 1378042) > +++ cpp/src/qpid/broker/SessionAdapter.cpp (working copy) > @@ -422,6 +422,10 @@ > if(!destination.empty() && state.exists(destination)) > throw NotAllowedException(QPID_MSG("Consumer tags must be unique")); > > + if (queue->getSettings().isBrowseOnly && acquireMode == 0) { > + QPID_LOG(info, "Overriding request to consume from browse only queue > " << queue->getName()); > + acquireMode = 1; > + } > // We allow browsing (acquireMode == 1) of exclusive queues, this is > required by HA. > if (queue->hasExclusiveOwner() && !queue->isExclusiveOwner(&session) && > acquireMode == 0) > throw ResourceLockedException(QPID_MSG("Cannot subscribe to > exclusive queue > > To me that seems like a cleaner an more direct implementation of the > functionality. > > > > - Gordon > > > On September 6th, 2012, 1:52 p.m., mick goulish wrote: > > Review request for qpid, Alan Conway, Gordon Sim, and Kenneth Giusti. > By mick goulish. > Updated Sept. 6, 2012, 1:52 p.m. > > > Description > browse-only queues > > prevent consumer from acquiring, and prevent queue from dequeing -- no change > to any class except Queue, and QueueSettings. > > > yes, questions: > > 1. is it OK to *not* change anything else. i.e. i don't see that > read-credit should be affected. > > 2. also, i did not want to change anything except Queue -- i.e. no change > to Consumer cursor. Let it think that it is consuming -- but Queue keeps > messages. This might allow queue to change back and forth from browse-only > to normal, if we ever care. > Testing > passed make check except for 1 HA test, on recent trunk > Create queue with "browse-only" arg, load 10 messages, and then see that > consumer can 'consume' any number of time, but same messages are always there. > > > > Here are some scripts for testing: > > ---- script: start_broker ------- > #! /bin/bash > > > PORT=5801 > TRUNK=/home/mick/redhat/trunk > QPIDD=${TRUNK}/qpid/cpp/src/qpidd > QPID_CONFIG=${TRUNK}/qpid/tools/src/py/qpid-config > SPOUT=${TRUNK}/qpid/cpp/examples/messaging/spout > DRAIN=${TRUNK}/qpid/cpp/examples/messaging/drain > LOG=/tmp/qpidd.log > > > rm -f ${LOG} > > > echo "Starting broker..." > ${QPIDD} \ > --auth=no \ > --log-enable info+ \ > --log-to-file ${LOG} \ > --log-hires-timestamp=yes \ > --log-source=yes \ > -p ${PORT} \ > -d > echo "Broker started." > ---- end script ----------------- > > > > ---- script: load_messages ------- > #! /bin/bash > > > PORT=5801 > TRUNK=/home/mick/redhat/trunk > QPIDD=${TRUNK}/qpid/cpp/src/qpidd > QPID_CONFIG=${TRUNK}/qpid/tools/src/py/qpid-config > SPOUT=${TRUNK}/qpid/cpp/examples/messaging/spout > DRAIN=${TRUNK}/qpid/cpp/examples/messaging/drain > LOG=/tmp/qpidd.log > > > echo "============================================" >> ${LOG} > echo "Loading the messages" >> ${LOG} > echo "============================================" >> ${LOG} > echo "Loading the messages" > > sleep 3 > > > # address was: 'q; {create: always, node:{ type: queue }}' > > COUNT=10 > echo "Sending $COUNT messages..." > ${SPOUT} \ > -c 10 \ > --broker "localhost:${PORT}" \ > --content "All Work and No Play Make Mick a Dull Boy" \ > 'q; {create: always, node:{type:queue , > x-declare:{arguments:{"browse-only":1}}}}' > > echo " " > echo "Done sending messages." > echo " " > ---- end script ------------------ > > > > > ---- script: consume ------------- > #! /bin/bash > > > PORT=5801 > TRUNK=/home/mick/redhat/trunk > QPIDD=${TRUNK}/qpid/cpp/src/qpidd > QPID_CONFIG=${TRUNK}/qpid/tools/src/py/qpid-config > SPOUT=${TRUNK}/qpid/cpp/examples/messaging/spout > DRAIN=${TRUNK}/qpid/cpp/examples/messaging/drain > LOG=/tmp/qpidd.log > > > echo "============================================" >> ${LOG} > echo "Consuming the messages" >> ${LOG} > echo "============================================" >> ${LOG} > echo "Consuming the messages" > > sleep 5 > > ${DRAIN} \ > --broker 'localhost:5801' \ > 'q;' > > > echo "done." > ---- end script ------------------ > > > ---- script: browse -------------- > #! /bin/bash > > > PORT=5801 > TRUNK=/home/mick/redhat/trunk > QPIDD=${TRUNK}/qpid/cpp/src/qpidd > QPID_CONFIG=${TRUNK}/qpid/tools/src/py/qpid-config > SPOUT=${TRUNK}/qpid/cpp/examples/messaging/spout > DRAIN=${TRUNK}/qpid/cpp/examples/messaging/drain > LOG=/tmp/qpidd.log > > echo "============================================" >> ${LOG} > echo "Browsing the messages" >> ${LOG} > echo "============================================" >> ${LOG} > echo "Browsing the messages" > > sleep 5 > > ${DRAIN} \ > --broker 'localhost:5801' \ > 'q; {mode: browse}' > > > echo "done." > ---- end script ------------------ > Diffs > * trunk/qpid/cpp/src/qpid/broker/Queue.cpp (1376905) > * trunk/qpid/cpp/src/qpid/broker/QueueSettings.h (1376905) > * trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp (1376905) > > View Diff > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
