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/10/02 22:36:44 UTC

Review Request: Prevent LinkRegistry from re-storing Links and Bridges during recovery.

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

Review request for qpid and Gordon Sim.

Description
-------

See https://issues.apache.org/jira/browse/QPID-4347

"The road to hell is paved..."

Hey Gordon, the patch you attached to the JIRA worked, but it wasn't obvious to me why. As a matter of fact, it wasn't obvious how I broke the code in the first place.

I think I understand now - my original change caused the Link and Bridge constructors to store their configurations whenever they were created. This store operation is fine when they are being created via management, but not when they are being re-created during recovery! This wasn't apparent to me when I refactored the code, resulting in the bug.

How do you feel about this patch as an alternative? Rather than rely on when the LinkRegistry's "store" member is initialized, the constructor specifically checks if it is being called during recovery and avoids the (re) store.

This addresses bug qpid-4347.
https://issues.apache.org/jira/browse/qpid-4347

Diffs
-----

/trunk/qpid/cpp/src/qpid/broker/Broker.h 1393152
/trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1393152
/trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1393152

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

Testing
-------

unit tests + a new test I'm adding to the store to check for this error.

Thanks,

Kenneth Giusti

Re: Review Request: Prevent LinkRegistry from re-storing Links and Bridges during recovery.

Posted by Gordon Sim <gs...@redhat.com>.

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

Ship it!


I agree, this is much clearer and explicit.

- Gordon Sim


On Oct. 2, 2012, 8:36 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7399/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2012, 8:36 p.m.)
> 
> 
> Review request for qpid and Gordon Sim.
> 
> 
> Description
> -------
> 
> See https://issues.apache.org/jira/browse/QPID-4347
> 
> "The road to hell is paved..."
> 
> Hey Gordon, the patch you attached to the JIRA worked, but it wasn't obvious to me why.   As a matter of fact, it wasn't obvious how I broke the code in the first place.
> 
> I think I understand now - my original change caused the Link and Bridge constructors to store their configurations whenever they were created.  This store operation is fine when they are being created via management, but not when they are being re-created during recovery!  This wasn't apparent to me when I refactored the code, resulting in the bug.
> 
> How do you feel about this patch as an alternative?  Rather than rely on when the LinkRegistry's "store" member is initialized, the constructor specifically checks if it is being called during recovery and avoids the (re) store.
> 
> 
> This addresses bug qpid-4347.
>     https://issues.apache.org/jira/browse/qpid-4347
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.h 1393152 
>   /trunk/qpid/cpp/src/qpid/broker/Broker.cpp 1393152 
>   /trunk/qpid/cpp/src/qpid/broker/LinkRegistry.cpp 1393152 
> 
> Diff: https://reviews.apache.org/r/7399/diff/
> 
> 
> Testing
> -------
> 
> unit tests + a new test I'm adding to the store to check for this error.
> 
> 
> Thanks,
> 
> Kenneth Giusti
> 
>