You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2012/01/19 15:22:46 UTC
Review Request: Modify Bridge & Link indexing to use explicit names.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3546/
-----------------------------------------------------------
Review request for qpid, Alan Conway, Gordon Sim, michael goulish, and Ted Ross.
Summary
-------
This patch modifies the way the broker's Link and Bridge objects are identified and managed. Specifically:
1) both Bridge and Links are now identified by explict names assigned by management, rather than destination host/port info.
- names beginning with the prefix "qpid." are reserved for qpidd internal use.
- for backward compatibility, if no name is assigned on creation, the broker will generate a name based on UUID
2) the corresponding QMF objects have been updated accordingly, with the additions of:
- the QMF Link object has been updated to provide a reference to the corresponding Connection
- the QMF Link object has been modified to allow the host/port/connectionRef to change on failover
- the QMF Bridge object has been modified to allow the Channel identifier to change (allowing Bridges to be reassigned to different links in the future)
3) Links/Bridges may now be created/deleted via the QMF Broker's generic "create" and "delete" methods
4) Some consolidation of the Link/Bridge creation APIs, specifically:
- Link/Bridges are created via calls to the LinkRegistry's "declare()" methods
- Link/Bridges are removed by calling their corresponding "destroy()" methods
More importantly, the above changes make it possible to create multiple Links between the same two brokers. This can be done by creating Links to the same destinations with different names. This is a change from the existing behavior, which uses the destination host/port as the unique Link identifier.
This addresses bug qpid-3767.
https://issues.apache.org/jira/browse/qpid-3767
Diffs
-----
/trunk/qpid/cpp/src/qpid/broker/Bridge.h 1233125
/trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1233125
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1233125
/trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1233125
/trunk/qpid/cpp/src/qpid/broker/Link.h 1233125
/trunk/qpid/cpp/src/qpid/broker/Link.cpp 1233125
/trunk/qpid/cpp/src/qpid/broker/LinkRegistry.h 1233125
/trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233125
/trunk/qpid/cpp/src/tests/federation.py 1233125
/trunk/qpid/specs/management-schema.xml 1233125
Diff: https://reviews.apache.org/r/3546/diff
Testing
-------
This patch fails to pass some of the cluster tests - I'm investigating this now. All non-cluster federation tests where passing (prior to my latest rebase).
Work remains, but I wanted to get this patch out for discussion before going much farther.
Thanks,
Kenneth
Re: Review Request: Modify Bridge & Link indexing to use explicit names.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3546/#review4469
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/Bridge.cpp
<https://reviews.apache.org/r/3546/#comment10022>
This changes the format of data as stored. Probably worth at least detecting that and giving a clearer message. (e.g. change the short string identifier to bridge-v2 and fail to load older bridge definitions)
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp
<https://reviews.apache.org/r/3546/#comment10033>
Given the indexing is changed, and previously I believe that was used to locate the link in order to create a bridge on it, I wonder whether this would break an old qpid-route for example?
To be clear that is not a real concern to me. I am fine with changing the schema in a non-backward compatible way. However it would be best to be explicit about it (e.g. simply fail on this method as unsupported) and if we were breaking the schema it might be worth seeing if there is anything else we want to change (i.e. bundle up some breaking changes).
/trunk/qpid/cpp/src/qpid/broker/Connection.cpp
<https://reviews.apache.org/r/3546/#comment10031>
assuming the reordering here is necessary, it might be worth a comment explaining why (to prevent it getting reordered incorrectly in any future changes)
/trunk/qpid/cpp/src/qpid/broker/Link.h
<https://reviews.apache.org/r/3546/#comment10032>
no need for const on return type as its not a reference
/trunk/qpid/specs/management-schema.xml
<https://reviews.apache.org/r/3546/#comment10035>
I'm not so keen on objIds... not saying it is wrong, just that its an aspect of QMf I feel uncomfortable with for broker management...
Being able to easily get at the connection currently in use by a link is certainly important though
- Gordon
On 2012-01-19 14:22:45, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3546/
> -----------------------------------------------------------
>
> (Updated 2012-01-19 14:22:45)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, michael goulish, and Ted Ross.
>
>
> Summary
> -------
>
> This patch modifies the way the broker's Link and Bridge objects are identified and managed. Specifically:
>
> 1) both Bridge and Links are now identified by explict names assigned by management, rather than destination host/port info.
> - names beginning with the prefix "qpid." are reserved for qpidd internal use.
> - for backward compatibility, if no name is assigned on creation, the broker will generate a name based on UUID
> 2) the corresponding QMF objects have been updated accordingly, with the additions of:
> - the QMF Link object has been updated to provide a reference to the corresponding Connection
> - the QMF Link object has been modified to allow the host/port/connectionRef to change on failover
> - the QMF Bridge object has been modified to allow the Channel identifier to change (allowing Bridges to be reassigned to different links in the future)
> 3) Links/Bridges may now be created/deleted via the QMF Broker's generic "create" and "delete" methods
> 4) Some consolidation of the Link/Bridge creation APIs, specifically:
> - Link/Bridges are created via calls to the LinkRegistry's "declare()" methods
> - Link/Bridges are removed by calling their corresponding "destroy()" methods
>
> More importantly, the above changes make it possible to create multiple Links between the same two brokers. This can be done by creating Links to the same destinations with different names. This is a change from the existing behavior, which uses the destination host/port as the unique Link identifier.
>
>
> This addresses bug qpid-3767.
> https://issues.apache.org/jira/browse/qpid-3767
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Link.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233125
> /trunk/qpid/cpp/src/tests/federation.py 1233125
> /trunk/qpid/specs/management-schema.xml 1233125
>
> Diff: https://reviews.apache.org/r/3546/diff
>
>
> Testing
> -------
>
> This patch fails to pass some of the cluster tests - I'm investigating this now. All non-cluster federation tests where passing (prior to my latest rebase).
>
> Work remains, but I wanted to get this patch out for discussion before going much farther.
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Modify Bridge & Link indexing to use explicit names.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3546/#review4470
-----------------------------------------------------------
Looks good overall.
/trunk/qpid/cpp/src/qpid/broker/Bridge.cpp
<https://reviews.apache.org/r/3546/#comment10023>
Is there a backward compatibility issue with adding the new fields at the front?
/trunk/qpid/cpp/src/qpid/broker/Link.cpp
<https://reviews.apache.org/r/3546/#comment10036>
I'm not keen on failing over in maintenance visit. That is potentially a long delay till a broker reconnects. I think we need to be able to trigger reconnects immediately on detecting a close.
/trunk/qpid/cpp/src/qpid/broker/Link.cpp
<https://reviews.apache.org/r/3546/#comment10034>
Not sure I understand the retry semantics, I think this can be simplified. E.g. if the next url isn't different you should keep going till you find one that is, not fall thru to the currentInterval logic. Likewise if next returns false, you should just keep going from the start, not fall thru.
/trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp
<https://reviews.apache.org/r/3546/#comment10040>
Throw rather than assert?
/trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp
<https://reviews.apache.org/r/3546/#comment10041>
ditto
/trunk/qpid/specs/management-schema.xml
<https://reviews.apache.org/r/3546/#comment10042>
Why is this RC rather than RO? Can you change durability?
- Alan
On 2012-01-19 14:22:45, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3546/
> -----------------------------------------------------------
>
> (Updated 2012-01-19 14:22:45)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, michael goulish, and Ted Ross.
>
>
> Summary
> -------
>
> This patch modifies the way the broker's Link and Bridge objects are identified and managed. Specifically:
>
> 1) both Bridge and Links are now identified by explict names assigned by management, rather than destination host/port info.
> - names beginning with the prefix "qpid." are reserved for qpidd internal use.
> - for backward compatibility, if no name is assigned on creation, the broker will generate a name based on UUID
> 2) the corresponding QMF objects have been updated accordingly, with the additions of:
> - the QMF Link object has been updated to provide a reference to the corresponding Connection
> - the QMF Link object has been modified to allow the host/port/connectionRef to change on failover
> - the QMF Bridge object has been modified to allow the Channel identifier to change (allowing Bridges to be reassigned to different links in the future)
> 3) Links/Bridges may now be created/deleted via the QMF Broker's generic "create" and "delete" methods
> 4) Some consolidation of the Link/Bridge creation APIs, specifically:
> - Link/Bridges are created via calls to the LinkRegistry's "declare()" methods
> - Link/Bridges are removed by calling their corresponding "destroy()" methods
>
> More importantly, the above changes make it possible to create multiple Links between the same two brokers. This can be done by creating Links to the same destinations with different names. This is a change from the existing behavior, which uses the destination host/port as the unique Link identifier.
>
>
> This addresses bug qpid-3767.
> https://issues.apache.org/jira/browse/qpid-3767
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Link.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233125
> /trunk/qpid/cpp/src/tests/federation.py 1233125
> /trunk/qpid/specs/management-schema.xml 1233125
>
> Diff: https://reviews.apache.org/r/3546/diff
>
>
> Testing
> -------
>
> This patch fails to pass some of the cluster tests - I'm investigating this now. All non-cluster federation tests where passing (prior to my latest rebase).
>
> Work remains, but I wanted to get this patch out for discussion before going much farther.
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Modify Bridge & Link indexing to use explicit names.
Posted by Ken Giusti <kg...@redhat.com>.
Hi Rob,
Absolutely correct on all points. There would certainly be work required on the Java broker side - for which I'd have to address before providing a more complete patch.
I should've been clearer in post - this patch is really preliminary. More of a proof of concept/proposal at this stage. Since it does result in a change to the user model, I thought it best to solicit wider input earlier in the process.
So, yes, no checking in at this time, but comments welcome!
thanks,
-K
----- Original Message -----
> Given that this change affects the management schema and what is
> effectively the primary key for bridges/links, this will obviously
> affect
> the Java broker too. (If checked in as is it would no doubt
> immediately
> break the java build).
>
> It's not that I personally disagree with the design decisions, but
> any
> change like this should probably be discussed and agreed widely
> before work
> is embarked upon... or else the person working on it should make the
> changes across *both* codebases.
>
> Cheers,
> Rob
>
> On 19 January 2012 15:22, Kenneth Giusti <kg...@apache.org> wrote:
>
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/3546/
> > -----------------------------------------------------------
> >
> > Review request for qpid, Alan Conway, Gordon Sim, michael goulish,
> > and Ted
> > Ross.
> >
> >
> > Summary
> > -------
> >
> > This patch modifies the way the broker's Link and Bridge objects
> > are
> > identified and managed. Specifically:
> >
> > 1) both Bridge and Links are now identified by explict names
> > assigned by
> > management, rather than destination host/port info.
> > - names beginning with the prefix "qpid." are reserved for qpidd
> > internal use.
> > - for backward compatibility, if no name is assigned on creation,
> > the
> > broker will generate a name based on UUID
> > 2) the corresponding QMF objects have been updated accordingly,
> > with the
> > additions of:
> > - the QMF Link object has been updated to provide a reference to
> > the
> > corresponding Connection
> > - the QMF Link object has been modified to allow the
> > host/port/connectionRef to change on failover
> > - the QMF Bridge object has been modified to allow the Channel
> > identifier to change (allowing Bridges to be reassigned to
> > different links
> > in the future)
> > 3) Links/Bridges may now be created/deleted via the QMF Broker's
> > generic
> > "create" and "delete" methods
> > 4) Some consolidation of the Link/Bridge creation APIs,
> > specifically:
> > - Link/Bridges are created via calls to the LinkRegistry's
> > "declare()"
> > methods
> > - Link/Bridges are removed by calling their corresponding
> > "destroy()"
> > methods
> >
> > More importantly, the above changes make it possible to create
> > multiple
> > Links between the same two brokers. This can be done by creating
> > Links to
> > the same destinations with different names. This is a change from
> > the
> > existing behavior, which uses the destination host/port as the
> > unique Link
> > identifier.
> >
> >
> > This addresses bug qpid-3767.
> > https://issues.apache.org/jira/browse/qpid-3767
> >
> >
> > Diffs
> > -----
> >
> > /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1233125
> > /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1233125
> > /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1233125
> > /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1233125
> > /trunk/qpid/cpp/src/qpid/broker/Link.h 1233125
> > /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1233125
> > /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.h 1233125
> > /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233125
> > /trunk/qpid/cpp/src/tests/federation.py 1233125
> > /trunk/qpid/specs/management-schema.xml 1233125
> >
> > Diff: https://reviews.apache.org/r/3546/diff
> >
> >
> > Testing
> > -------
> >
> > This patch fails to pass some of the cluster tests - I'm
> > investigating
> > this now. All non-cluster federation tests where passing (prior to
> > my
> > latest rebase).
> >
> > Work remains, but I wanted to get this patch out for discussion
> > before
> > going much farther.
> >
> >
> > Thanks,
> >
> > Kenneth
> >
> >
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Review Request: Modify Bridge & Link indexing to use explicit names.
Posted by Rob Godfrey <ro...@gmail.com>.
Given that this change affects the management schema and what is
effectively the primary key for bridges/links, this will obviously affect
the Java broker too. (If checked in as is it would no doubt immediately
break the java build).
It's not that I personally disagree with the design decisions, but any
change like this should probably be discussed and agreed widely before work
is embarked upon... or else the person working on it should make the
changes across *both* codebases.
Cheers,
Rob
On 19 January 2012 15:22, Kenneth Giusti <kg...@apache.org> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3546/
> -----------------------------------------------------------
>
> Review request for qpid, Alan Conway, Gordon Sim, michael goulish, and Ted
> Ross.
>
>
> Summary
> -------
>
> This patch modifies the way the broker's Link and Bridge objects are
> identified and managed. Specifically:
>
> 1) both Bridge and Links are now identified by explict names assigned by
> management, rather than destination host/port info.
> - names beginning with the prefix "qpid." are reserved for qpidd
> internal use.
> - for backward compatibility, if no name is assigned on creation, the
> broker will generate a name based on UUID
> 2) the corresponding QMF objects have been updated accordingly, with the
> additions of:
> - the QMF Link object has been updated to provide a reference to the
> corresponding Connection
> - the QMF Link object has been modified to allow the
> host/port/connectionRef to change on failover
> - the QMF Bridge object has been modified to allow the Channel
> identifier to change (allowing Bridges to be reassigned to different links
> in the future)
> 3) Links/Bridges may now be created/deleted via the QMF Broker's generic
> "create" and "delete" methods
> 4) Some consolidation of the Link/Bridge creation APIs, specifically:
> - Link/Bridges are created via calls to the LinkRegistry's "declare()"
> methods
> - Link/Bridges are removed by calling their corresponding "destroy()"
> methods
>
> More importantly, the above changes make it possible to create multiple
> Links between the same two brokers. This can be done by creating Links to
> the same destinations with different names. This is a change from the
> existing behavior, which uses the destination host/port as the unique Link
> identifier.
>
>
> This addresses bug qpid-3767.
> https://issues.apache.org/jira/browse/qpid-3767
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/broker/Bridge.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/Bridge.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Connection.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/Link.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/Link.cpp 1233125
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.h 1233125
> /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1233125
> /trunk/qpid/cpp/src/tests/federation.py 1233125
> /trunk/qpid/specs/management-schema.xml 1233125
>
> Diff: https://reviews.apache.org/r/3546/diff
>
>
> Testing
> -------
>
> This patch fails to pass some of the cluster tests - I'm investigating
> this now. All non-cluster federation tests where passing (prior to my
> latest rebase).
>
> Work remains, but I wanted to get this patch out for discussion before
> going much farther.
>
>
> Thanks,
>
> Kenneth
>
>