qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: Review Request: browse-only queues
Date Thu, 06 Sep 2012 17:56:59 GMT
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: dev-unsubscribe@qpid.apache.org
> For additional commands, e-mail: dev-help@qpid.apache.org
> 



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org


Mime
View raw message