> On June 20, 2013, 5:12 p.m., Andrew Stitcher wrote: > > I really like a lot of this change. > > > > I'm a little cautious about the new ConnectionControl interface: > > > > I think that the new "ConnectionControl" interface is really what would be > > the protocol independent broker::Connection interface. > > > > So I'd prefer to see the existing broker::Connection class moved to > > broker::amqp_0_10::Connection, and this new interface called > > broker::Connection. This would provide a useful (if small) protocol > > independent cleanup of the broker code. > > > > I think this would give a similar (or smaller) number of renamings through > > the tree. > > > > (Incidentally I think that *Control and *Identity are not great examples of > >
[Ignore that last line - I deleted it and forgot to save it before publishing the review] - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11997/#review22168 ----------------------------------------------------------- On June 20, 2013, 2:51 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, 2:51 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/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/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 > >
