On Thu, 2012-09-06 at 13:24 -0400, mick wrote: > 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 ?
I think it's safer and cleaner to say a browse-only queue is one that allows only browsers. We already have code paths to distinguish browsers vs. consumers, I think it's better to use the existing browser path than create a 3rd path that makes consumers sometimes browse. As for future changes, I don't think we can say now what will be best then, but in general an approach involves less code will be easier to maintain. > > 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] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
