----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4846/#review7174 -----------------------------------------------------------
Looks good in general, a couple comments - I'm most concerned about the hard-coded channel. /trunk/qpid/cpp/src/qpid/broker/Link.h <https://reviews.apache.org/r/4846/#comment15824> nit: suggest naming "failoverExchange" or something similarly descriptive /trunk/qpid/cpp/src/qpid/broker/Link.cpp <https://reviews.apache.org/r/4846/#comment15825> why do we want to reserve a channel rather than assign one in the normal course of opening a session? Seems error prone. /trunk/qpid/cpp/src/qpid/broker/Link.cpp <https://reviews.apache.org/r/4846/#comment15826> Might be worth merging the addresses to eliminate duplicates. I actually though I had already done in this but it doesn't appear so. I dreamed that I added Url::merge(const Address&) Url::merge(const Url&) /trunk/qpid/cpp/src/qpid/broker/Link.cpp <https://reviews.apache.org/r/4846/#comment15827> Aside: I'm not sure what "closed by management" means. Do we actually know the link dtor is only called as a result of a management command? /trunk/qpid/cpp/src/qpid/broker/Link.cpp <https://reviews.apache.org/r/4846/#comment15828> I don't think you need to do anything here. /trunk/qpid/cpp/src/qpid/broker/Link.cpp <https://reviews.apache.org/r/4846/#comment15829> Aside again: this pattern pops up a couple of times now, can we abstract a little "broker subscription toolkit" to simplify this pattern & put the common logic in one place? /trunk/qpid/cpp/src/qpid/broker/Link.cpp <https://reviews.apache.org/r/4846/#comment15830> I'm definitely queasy about this hard-coded channel number. The session code doesn't know about it and could re-use it. - Alan On 2012-04-23 20:57:48, Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4846/ > ----------------------------------------------------------- > > (Updated 2012-04-23 20:57:48) > > > Review request for qpid, Alan Conway and Gordon Sim. > > > Summary > ------- > > Still a WIP, but I wanted early feedback as I'm not too experienced with the > subscription management code involved (completely stolen from Alan). > > This patch allows the Link to subscribe to the remote broker's amq.failover > exchange - if it exists. This allows the Link to be updated dynamically > should the remote broker be part of a cluster, and the cluster membership > changes. > > Light testing against a cluster confirms that this patch fixes qpid-3963. > Testing against a non-cluster remote causes the remote to log the following > error, but otherwise behaves ok: > > 2012-04-23 16:45:27 error Execution exception: not-found: Exchange not found: > amq.failover (../../../qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp:101) > > > This addresses bug qpid-3963. > https://issues.apache.org/jira/browse/qpid-3963 > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/Link.h 1329301 > /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1329301 > > Diff: https://reviews.apache.org/r/4846/diff > > > Testing > ------- > > minimal. > > > Thanks, > > Kenneth > >
