----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11997/#review22168 -----------------------------------------------------------
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 /trunk/qpid/cpp/src/qpid/broker/ConnectionIdentity.h <https://reviews.apache.org/r/11997/#comment45564> This line makes ConnectionIdentity amqp 0_10 specific - surely that's not intended? Also it doesn't seem to be an identity type of functionality/property. (I also can't see where it is actually used.) /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp <https://reviews.apache.org/r/11997/#comment45563> I think this dynamic_cast is a little awkward, but I can't think of a short neater solution. /trunk/qpid/cpp/src/qpid/ha/ConnectionObserver.cpp <https://reviews.apache.org/r/11997/#comment45565> 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. - Andrew Stitcher 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 > >
