> On June 20, 2013, 5:12 p.m., Andrew Stitcher wrote: > > /trunk/qpid/cpp/src/qpid/ha/ConnectionObserver.cpp, line 86 > > <https://reviews.apache.org/r/11997/diff/1/?file=309179#file309179line86> > > > > Was this a deliberate deletion? > > > > It doesn't seem to be explained as part of the overall change? > > > > If this is a bug fix then I thin kit should be called out specifically. > > Gordon Sim wrote: > Oops! Well spotted, that is unintended. It was an artefact of an initial > attempt to avoid the abort() call here by throwing an exception (as suggested > in the current doc for the ConnectionObserver interface), but that doesn't > actually work as the HA plugin holds a pointer to the object passed in for > aborting at a later stage in some cases. I'll fix that.
I've created a new review for adding connection approval to 1.0 path that includes this refactoring and also moves qpid::broker::Connection to qpid::broker::amqp_0_10::Connection. See https://reviews.apache.org/r/12029/. This review will now be closed. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11997/#review22168 ----------------------------------------------------------- On June 20, 2013, 6:04 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11997/ > ----------------------------------------------------------- > > (Updated June 20, 2013, 6:04 p.m.) > > > Review request for qpid, Andrew Stitcher, Alan Conway, Kenneth Giusti, Ted > Ross, and Chug Rolke. > > > Description > ------- > > In preparation for authorisation in AMQP 1.0, I needed to decouple > ConnectionObserver from the 0-10 specific Connection class: > > * combined Connection::getMgmtId() and Connection::getUrl() as it turns out > they are the same thing (though stored twice); I kept the getMgmtId() as the > name as I think its more correct and informative as to purpose > * add a couple of methods - isLink() and getClientProperties() - to > ConnectionIdentity (which seem reasonably in keeping with the current scope > of that interface) > * added new ConnectionControl interface extending ConnectionIdentity and use > this in place of broker::Connection itself in ConnectionObserver (at present > this just adds one method, abort()) > > Most of the rest of the changes merely update the implementations of > ConnectionObserver accordingly. Other changes: > > * have ConnectionIdentity of publisher returned by pointer from Message since > it may not be set > * remove Message::getPublisherOwnership(), add > Message::isLocalTo(OwnershipToken*) instead > * remove Message::getPublisherUserId(), Message::getPublisherObjectId(), > Message::getPublisherUrl(); these essentially duplicate the same methods on > ConnectionIdentity, are used only by ManagementAgent and (particularly the > latter two) really only make sense in that context anyway and I'm keen to > keep Message as clean as possible > > > This addresses bug QPID-4712. > https://issues.apache.org/jira/browse/QPID-4712 > > > Diffs > ----- > > /trunk/qpid/cpp/src/Makefile.am 1494646 > /trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.h 1494646 > /trunk/qpid/cpp/src/qpid/acl/AclConnectionCounter.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/Connection.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/ConnectionControl.h PRE-CREATION > /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/ConnectionIdentity.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/ConnectionObserver.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/ConnectionObservers.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/Message.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/Queue.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/SemanticState.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/SessionAdapter.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/Connection.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/Interconnect.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedConnection.h 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/ManagedConnection.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/Sasl.cpp 1494646 > /trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp 1494646 > /trunk/qpid/cpp/src/qpid/ha/BackupConnectionExcluder.h 1494646 > /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.h 1494646 > /trunk/qpid/cpp/src/qpid/ha/BrokerReplicator.cpp 1494646 > /trunk/qpid/cpp/src/qpid/ha/ConnectionObserver.h 1494646 > /trunk/qpid/cpp/src/qpid/ha/ConnectionObserver.cpp 1494646 > /trunk/qpid/cpp/src/qpid/ha/Primary.h 1494646 > /trunk/qpid/cpp/src/qpid/ha/Primary.cpp 1494646 > /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.h 1494646 > /trunk/qpid/cpp/src/qpid/ha/RemoteBackup.cpp 1494646 > /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1494646 > > Diff: https://reviews.apache.org/r/11997/diff/ > > > Testing > ------- > > all tests pass > > > Thanks, > > Gordon Sim > >
