qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Stitcher" <astitc...@apache.org>
Subject Re: Review Request: allow ConnectionObserver to observe connections for different protocol versions
Date Thu, 20 Jun 2013 17:17:58 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message