----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5253/#review8201 -----------------------------------------------------------
Ship it! I have no real issue with this, it is no worse than what is already there (the lifecycle management is rather messy to begin with). A couple of suggestions included below, but these would be entirely optional. What I would really like would be a more wholesale cleanup of object lifecycle, unifying this sort of pattern with store, management etc. That however is certainly beyond the cope of this change. Out of curiousity, why do you need this information in the HA plugin? /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/5253/#comment17745> Though I understand why this is here, I wonder if it might not be better moved to QueueRegistry::destroy(). The justification being that removal from the QueueRegistry is the outwardly visible action (the rest is internal cleanup). Having the create and destroy communicated both by the registry would be a little cleaner in my view. /trunk/qpid/cpp/src/qpid/broker/Queue.cpp <https://reviews.apache.org/r/5253/#comment17744> I think it would be a little clearer if this were moved to Broker::bind(). That corresponds more directly to the equivalent for unbind() - Gordon On 2012-05-28 21:35:05, Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5253/ > ----------------------------------------------------------- > > (Updated 2012-05-28 21:35:05) > > > Review request for qpid, Andrew Stitcher and Gordon Sim. > > > Summary > ------- > > The HA plugin needs to know when queues are created. This patch creates a > ConfigurationObserver to observe queue and exchange create/delete and > bind/unbind. > The MessageStore API has these hooks but you can't have multiple > MessageStores on a broker, also calls to the MessageStore interface are > conditional on durability which is not what I want for HA. The > ConfigurationObserver calls are placed close to the corresponding > MessageStore calls, except that the create() calls are placed earlier - just > before the queue/exchange is stored in the Registry. That allows an Observer > to effectively abort the creation of the queue/exchange (HA doesn't actually > need this but it seemed like it might be useful) > > How does this look? Any suggestions for better ways to do it? > > > Diffs > ----- > > /trunk/qpid/cpp/src/Makefile.am 1343351 > /trunk/qpid/cpp/src/qpid/broker/Broker.h 1343351 > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1343351 > /trunk/qpid/cpp/src/qpid/broker/ConfigurationObserver.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/ConfigurationObservers.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1343351 > /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1343351 > /trunk/qpid/cpp/src/qpid/broker/Observers.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1343351 > /trunk/qpid/cpp/src/qpid/broker/QueueRegistry.cpp 1343351 > > Diff: https://reviews.apache.org/r/5253/diff > > > Testing > ------- > > make check > > > Thanks, > > Alan > >
