Qpid › Apache Qpid developers

Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

Classic

List

Threaded

6 messages Options

kgiusti

Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4846/
-----------------------------------------------------------

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

Gordon Sim

Re: Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

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

Alan Conway

Re: Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

In reply to this post by kgiusti

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

kgiusti

Re: Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

In reply to this post by kgiusti

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4846/
-----------------------------------------------------------

(Updated 2012-04-26 19:19:31.447871)

Review request for qpid, Alan Conway and Gordon Sim.

Changes
-------

This patch should be final - I've got more testing to do and that might result in some changes, but consider this my solution for QPID-3963.

I also will be adding a unit test to verify the fix, but that is TBD.

In summary:

1) Each broker Link attempts to subscribe to the amq.failover exchange on the remote
2) The set of failover URLs learned from the remote are replicated on the local cluster when a new member is added.

I've also tried to apply most of the comments from the last review.

Thanks, -K

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 (updated)
-----

/trunk/qpid/cpp/xml/cluster.xml 1329301
/trunk/qpid/cpp/src/qpid/cluster/Connection.h 1329301
/trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1329301
/trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1329301
/trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1329301
/trunk/qpid/cpp/src/qpid/broker/Link.cpp 1329301
/trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1329301
/trunk/qpid/cpp/src/qpid/broker/Link.h 1329301

Diff: https://reviews.apache.org/r/4846/diff

Testing
-------

minimal.

Thanks,

Kenneth

Gordon Sim

Re: Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4846/#review7376
-----------------------------------------------------------

Ship it!

/trunk/qpid/cpp/xml/cluster.xml
<https://reviews.apache.org/r/4846/#comment16280>

I like this!

- Gordon

On 2012-04-26 19:19:31, Kenneth Giusti wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4846/
> -----------------------------------------------------------
>
> (Updated 2012-04-26 19:19:31)
>
>
> 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/xml/cluster.xml 1329301
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1329301
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1329301
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/Link.h 1329301
>
> Diff: https://reviews.apache.org/r/4846/diff
>
>
> Testing
> -------
>
> minimal.
>
>
> Thanks,
>
> Kenneth
>
>

Alan Conway

Re: Review Request: QPID-3963: have the Link monitor the remote broker's amq.failover exchange, and use this information during failover.

In reply to this post by kgiusti

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4846/
> -----------------------------------------------------------
>
> (Updated 2012-04-26 19:19:31)
>
>
> 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/xml/cluster.xml 1329301
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1329301
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1329301
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/ExchangeRegistry.cpp 1329301
> /trunk/qpid/cpp/src/qpid/broker/Link.h 1329301
>
> Diff: https://reviews.apache.org/r/4846/diff
>
>
> Testing
> -------
>
> minimal.
>
>
> Thanks,
>
> Kenneth
>
>