2009/9/22 Rajith Attapattu <[email protected]>:
> 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.
I still beleieve the better option is to put the logic inside
failoverAllowed and have it return false. That way we can guarrantee
getNextBrokerDetails never returns null when failover is allowed to
take place.
Cheers
Martin
>> 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]
>
>
--
Martin Ritchie
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:[email protected]
