On Tue, Sep 22, 2009 at 11:18 AM, Martin Ritchie <[email protected]> wrote:
> 2009/9/22 Rajith Attapattu <[email protected]>:
>> On Tue, Sep 22, 2009 at 8:58 AM, Martin Ritchie <[email protected]> wrote:
>>> 2009/9/22 Rajith Attapattu <[email protected]>:
>>>> On Tue, Sep 22, 2009 at 7:13 AM, Martin Ritchie <[email protected]>
>>>> wrote:
>>>>> Hi Rajith,
>>>>> Will this not cause both the 0-8 and 0-10 code path to throw a
>>>>> NullPointer if none of the brokers are available on startup.
>>>>> Is the FailoverPolicy cycle count not what stops the looping?
>>>>>
>>>>> Regards
>>>>
>>>> Hi Martin,
>>>>
>>>> If the initial broker is not available it will throw
>>>> ConnectionException. My title for the JIRA is a bit misleading here.
>>>> If the initial connection is successful and if it gets disconnected
>>>> before it gets a chance to update its membership list via the failover
>>>> exchange this looping can happen.
>>>>
>>>> The 0-8 codepath and a non-clustered 0-10 codepath should not be using
>>>> failover exchange method.
>>>> Perhaps we should add a check for this??
>>>>
>>>> This modification is done only for the FailoverExchange method where
>>>> cycle count is not applicable.
>>>> The failover exchange maintains a dynamic list of brokers based on the
>>>> updates send by the failover exchange it is subscribed to.
>>>> It will continue until total cluster failure.
>>>>
>>>> Regards,
>>>>
>>>> Rajith
>>>
>>> I see the dynamic list bits now but I'm still don't see how returning
>>> null is checked? In the failover mode the returned value from
>>> getNextBroker is passed straight in to makeBrokerConnection which
>>> assumes the BrokerDetails is not null.
>>>
>>> I must be missing something.
>>>
>>> Martin
>>
>> In the initial connection it checks if brokerDetails is null
>>
>> while (!_connected && retryAllowed && brokerDetails != null)
>>
>> and during failover we do the following.
>> public boolean attemptReconnection()
>> {
>> BrokerDetails broker = null;
>> while (_failoverPolicy.failoverAllowed() && (broker =
>> _failoverPolicy.getNextBrokerDetails()) != null)
>
> Are you sure you've commited all the code?
Ah I have forgotten to commit this bit last night: (:
Thx for questioning this.
The null check was added bcos both FailoverExchangeMethod and
FailoverRoundRobinServers could return null for getNextBrokerDetails.
> AMQConnection:L515 there is no null check.
> while (!_connected && retryAllowed)
> ....
> retryAllowed = _failoverPolicy.failoverAllowed();
> brokerDetails = _failoverPolicy.getNextBrokerDetails();
>
> AMQConnection: L:692 doesn't do a null check.
> public boolean attemptReconnection()
> {
> while (_failoverPolicy.failoverAllowed())
> {
> try
> {
> makeBrokerConnection(_failoverPolicy.getNextBrokerDetails());
>
> I'm also not sure that we should have to change all our checks from
> while (_failoverPolicy.failoverAllowed())
>
> to
> while (_failoverPolicy.failoverAllowed() && (broker =
> _failoverPolicy.getNextBrokerDetails()) != null)
>
> If you are changing the condition of failoverAllowed to require
> _failoverPolicy.getNextBrokerDetails() to be non-null I'd add that as
> part of the FailoverExchangeMethod's getNextBrokerDetails().
I'd have to go through the code again and see how this can be done.
Most likely this is doable.
>> On a separate note, I appreciate your questions. It is best some sort
>> of review is done for most of the commits.
>> If you aren't clear on something please don't hesitate to ask as it
>> could potentially be an issue.
>
> I try and review all the Java commits but there are so many. I too
> appreciate people reviewing my work. We all make mistakes from time to
> time, well except Rafi ;). I think it just helps us write better code,
> if it is reviewed.
>
> Cheers
>
> martin
>
>>>> ---------------------------------------------------------------------
>>>> Apache Qpid - AMQP Messaging Implementation
>>>> Project: http://qpid.apache.org
>>>> Use/Interact: mailto:[email protected]
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Martin Ritchie
>>>
>>> ---------------------------------------------------------------------
>>> Apache Qpid - AMQP Messaging Implementation
>>> Project: http://qpid.apache.org
>>> Use/Interact: mailto:[email protected]
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> Apache Qpid - AMQP Messaging Implementation
>> Project: http://qpid.apache.org
>> Use/Interact: mailto:[email protected]
>>
>>
>
>
>
> --
> Martin Ritchie
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:[email protected]
>
>
--
Regards,
Rajith Attapattu
Red Hat
http://rajith.2rlabs.com/
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
