I agree with you that "NoFailover" is redundent and I suggest we remove it. Currently the AMQConnection defaults to FailoverSingleServer by default if only one broker is specified in the URL. In most situations I have come across users prefer not to reconnect by default, so IMO we should not retry. The other option is to make NoFailover the default, but then if people want to retry they will have to explicitly specify FailoverSingleServer as the failover method which is not ideal.
Most end users expect safe default behaviour. Retrying without explicit instructions is not a good thing from my experience. So I think the best solution is to leave things as it is and explicitly specify retries in the URL if you want retries. Regards, Rajith On Wed, Jun 24, 2009 at 11:49 AM, Martin Ritchie<ritch...@apache.org> wrote: > I'd have to question what the point of the class is then as it is it > is basically a much more complicated version of 'NoFailover'. > > It should at least allow failover to occur once. If not once per > successfull connection > > Martin > > 2009/6/24 Rajith Attapattu <rajit...@gmail.com>: >> Martin, >> >> When we discussed this issue we agreed that by default the >> SingleFailoverMethod should not retry at all. >> So in order to make the initial connection attempt work, we made >> AMQConnection use getCurrentBrokerDetails() instead of the >> getNextBrokerDetails() to get the initial broker to connect. >> >> Regards, >> >> Rajith >> >> On Wed, Jun 24, 2009 at 11:27 AM, Martin Ritchie<ritch...@apache.org> wrote: >>> Hi Rajith, >>> >>> Why did you set the default retries to 0 as well as starting the >>> _currentRetries from 0? I thought the original problem was that it >>> would always try twice with this commit we effectively reduced the >>> retries by 2. Do you have time to write a test to ensure that >>> FailoverSingleServer only retires once not twice :) >>> >>>> /** The default number of times to rety a conection to this server */ >>>> - public static final int DEFAULT_SERVER_RETRIES = 1; >>>> + public static final int DEFAULT_SERVER_RETRIES = 0; >>> >>> This commit has resulted in the SingleServerFailover not working. >>> >>> When I make my connection failover I get : >>> >>> main 2009-06-24 16:21:45,105 DEBUG >>> [qpid.client.protocol.AMQProtocolHandler] Session closed called with >>> failover state currently FailoverState: NOT STARTED >>> main 2009-06-24 16:21:45,105 DEBUG [apache.qpid.jms.FailoverPolicy] >>> All failover methods exhausted >>> main 2009-06-24 16:21:45,106 DEBUG >>> [qpid.client.protocol.AMQProtocolHandler] Failover not allowed by >>> policy. >>> main 2009-06-24 16:21:45,106 DEBUG [apache.qpid.jms.FailoverPolicy] >>> All failover methods exhausted >>> main 2009-06-24 16:21:45,106 DEBUG >>> [qpid.client.protocol.AMQProtocolHandler] Failover Policy: >>> Failover not allowed >>> Failover policy methods >>>>Single Server: >>> Max Retries:0 >>> Current Retry:0 >>> tcp://localhost:5672 >>> >>> As the current retry is 0 and the Max is 0 then failover doesn't occur. >>> >>> Am I missing something? >>> >>> Cheers >>> >>> Martin >>> >>> 2009/5/14 <raj...@apache.org>: >>>> Author: rajith >>>> Date: Thu May 14 19:50:23 2009 >>>> New Revision: 774899 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=774899&view=rev >>>> Log: >>>> This is a fix for QPID-1859 >>>> For FailoverSingleServer the default for retries is set to 0 and the >>>> current_retries start from 0 instead of -1. >>>> For FailoverRoundRobinServers the current_broker_index now starts from 0 >>>> instead of -1. >>>> The AMQConnection now uses the getCurrentBrokerDetails() instead of the >>>> getNextBrokerDetails() to get the initial broker to connect. >>>> >>>> Modified: >>>> >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java >>>> >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverRoundRobinServers.java >>>> >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverSingleServer.java >>>> >>>> Modified: >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java >>>> URL: >>>> http://svn.apache.org/viewvc/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java?rev=774899&r1=774898&r2=774899&view=diff >>>> ============================================================================== >>>> --- >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java >>>> (original) >>>> +++ >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java >>>> Thu May 14 19:50:23 2009 >>>> @@ -455,7 +455,7 @@ >>>> } >>>> >>>> _failoverPolicy = new FailoverPolicy(connectionURL, this); >>>> - BrokerDetails brokerDetails = >>>> _failoverPolicy.getNextBrokerDetails(); >>>> + BrokerDetails brokerDetails = >>>> _failoverPolicy.getCurrentBrokerDetails(); >>>> if (brokerDetails.getTransport().equals(BrokerDetails.VM)) >>>> { >>>> _delegate = new AMQConnectionDelegate_8_0(this); >>>> >>>> Modified: >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverRoundRobinServers.java >>>> URL: >>>> http://svn.apache.org/viewvc/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverRoundRobinServers.java?rev=774899&r1=774898&r2=774899&view=diff >>>> ============================================================================== >>>> --- >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverRoundRobinServers.java >>>> (original) >>>> +++ >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverRoundRobinServers.java >>>> Thu May 14 19:50:23 2009 >>>> @@ -35,19 +35,19 @@ >>>> public static final int DEFAULT_SERVER_RETRIES = 0; >>>> >>>> /** The index into the hostDetails array of the broker to which we are >>>> connected */ >>>> - private int _currentBrokerIndex = -1; >>>> + private int _currentBrokerIndex = 0; >>>> >>>> /** The number of times to retry connecting for each server */ >>>> private int _serverRetries; >>>> >>>> /** The current number of retry attempts made */ >>>> - private int _currentServerRetry; >>>> + private int _currentServerRetry = 0; >>>> >>>> /** The number of times to cycle through the servers */ >>>> private int _cycleRetries; >>>> >>>> /** The current number of cycles performed. */ >>>> - private int _currentCycleRetries; >>>> + private int _currentCycleRetries = 0; >>>> >>>> /** Array of BrokerDetail used to make connections. */ >>>> protected ConnectionURL _connectionDetails; >>>> @@ -62,7 +62,7 @@ >>>> _connectionDetails = connectionDetails; >>>> >>>> // There is no current broker at startup so set it to -1. >>>> - _currentBrokerIndex = -1; >>>> + _currentBrokerIndex = 0; >>>> >>>> String cycleRetries = >>>> _connectionDetails.getFailoverOption(ConnectionURL.OPTIONS_FAILOVER_CYCLE); >>>> >>>> @@ -83,18 +83,21 @@ >>>> _currentCycleRetries = 0; >>>> >>>> _serverRetries = 0; >>>> - _currentServerRetry = -1; >>>> + _currentServerRetry = 0; >>>> } >>>> >>>> public void reset() >>>> { >>>> _currentBrokerIndex = 0; >>>> _currentCycleRetries = 0; >>>> - _currentServerRetry = -1; >>>> + _currentServerRetry = 0; >>>> } >>>> >>>> public boolean failoverAllowed() >>>> { >>>> + System.out.println("===================================="); >>>> + System.out.println(toString()); >>>> + System.out.println("===================================="); >>>> return ((_currentCycleRetries < _cycleRetries) || >>>> (_currentServerRetry < _serverRetries)); >>>> //|| (_currentBrokerIndex <= >>>> (_connectionDetails.getBrokerCount() - 1))); >>>> } >>>> @@ -102,16 +105,11 @@ >>>> public void attainedConnection() >>>> { >>>> _currentCycleRetries = 0; >>>> - _currentServerRetry = -1; >>>> + _currentServerRetry = 0; >>>> } >>>> >>>> public BrokerDetails getCurrentBrokerDetails() >>>> { >>>> - if (_currentBrokerIndex == -1) >>>> - { >>>> - return null; >>>> - } >>>> - >>>> return _connectionDetails.getBrokerDetails(_currentBrokerIndex); >>>> } >>>> >>>> @@ -123,20 +121,8 @@ >>>> { >>>> if (_currentServerRetry < _serverRetries) >>>> { >>>> - if (_currentBrokerIndex == -1) >>>> - { >>>> - _currentBrokerIndex = 0; >>>> - >>>> - >>>> setBroker(_connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> - >>>> - _logger.info("First run using " + >>>> _connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> - } >>>> - else >>>> - { >>>> - _logger.info("Retrying " + >>>> _connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> - doDelay=true; >>>> - } >>>> - >>>> + _logger.info("Trying " + >>>> _connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> + doDelay= _currentBrokerIndex != 0; >>>> _currentServerRetry++; >>>> } >>>> else >>>> @@ -156,19 +142,8 @@ >>>> { >>>> if (_currentServerRetry < _serverRetries) >>>> { >>>> - if (_currentBrokerIndex == -1) >>>> - { >>>> - _currentBrokerIndex = 0; >>>> - >>>> - >>>> setBroker(_connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> - >>>> - _logger.info("First run using " + >>>> _connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> - } >>>> - else >>>> - { >>>> - _logger.info("Retrying " + >>>> _connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> - doDelay=true; >>>> - } >>>> + _logger.info("Trying " + >>>> _connectionDetails.getBrokerDetails(_currentBrokerIndex)); >>>> + doDelay= _currentBrokerIndex != 0; >>>> >>>> _currentServerRetry++; >>>> } >>>> @@ -227,7 +202,7 @@ >>>> } >>>> } >>>> >>>> - _currentServerRetry = -1; >>>> + _currentServerRetry = 0; >>>> _currentBrokerIndex = index; >>>> } >>>> >>>> >>>> Modified: >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverSingleServer.java >>>> URL: >>>> http://svn.apache.org/viewvc/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverSingleServer.java?rev=774899&r1=774898&r2=774899&view=diff >>>> ============================================================================== >>>> --- >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverSingleServer.java >>>> (original) >>>> +++ >>>> qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/jms/failover/FailoverSingleServer.java >>>> Thu May 14 19:50:23 2009 >>>> @@ -30,7 +30,7 @@ >>>> private static final Logger _logger = >>>> LoggerFactory.getLogger(FailoverSingleServer.class); >>>> >>>> /** The default number of times to rety a conection to this server */ >>>> - public static final int DEFAULT_SERVER_RETRIES = 1; >>>> + public static final int DEFAULT_SERVER_RETRIES = 0; >>>> >>>> /** The details of the Single Server */ >>>> private BrokerDetails _brokerDetail; >>>> @@ -39,7 +39,7 @@ >>>> protected int _retries; >>>> >>>> /** The current number of attempts made to the server */ >>>> - protected int _currentRetries; >>>> + protected int _currentRetries = 0; >>>> >>>> >>>> public FailoverSingleServer(ConnectionURL connectionDetails) >>>> @@ -61,7 +61,7 @@ >>>> >>>> public void reset() >>>> { >>>> - _currentRetries = -1; >>>> + _currentRetries = 0; >>>> } >>>> >>>> public boolean failoverAllowed() >>>> >>>> >>>> >>>> --------------------------------------------------------------------- >>>> Apache Qpid - AMQP Messaging Implementation >>>> Project: http://qpid.apache.org >>>> Use/Interact: mailto:commits-subscr...@qpid.apache.org >>>> >>>> >>> >>> >>> >>> -- >>> Martin Ritchie >>> >>> --------------------------------------------------------------------- >>> Apache Qpid - AMQP Messaging Implementation >>> Project: http://qpid.apache.org >>> Use/Interact: mailto:dev-subscr...@qpid.apache.org >>> >>> >> >> >> >> -- >> Regards, >> >> Rajith Attapattu >> Red Hat >> http://rajith.2rlabs.com/ >> >> --------------------------------------------------------------------- >> Apache Qpid - AMQP Messaging Implementation >> Project: http://qpid.apache.org >> Use/Interact: mailto:dev-subscr...@qpid.apache.org >> >> > > > > -- > Martin Ritchie > > --------------------------------------------------------------------- > Apache Qpid - AMQP Messaging Implementation > Project: http://qpid.apache.org > Use/Interact: mailto:dev-subscr...@qpid.apache.org > > -- Regards, Rajith Attapattu Red Hat http://rajith.2rlabs.com/ --------------------------------------------------------------------- Apache Qpid - AMQP Messaging Implementation Project: http://qpid.apache.org Use/Interact: mailto:dev-subscr...@qpid.apache.org
