NMS: Small fix for newly implemented failover support

[Daniel Ellis <ma...@danellis.co.uk>]

Good work getting the new failover transport in to NMS, which is much
appreciated and it looks like a substantial piece of work. I have been
testing the new failover transport, and auto reconnection seems fine by
adding the same server twice using a connection uri such as:

  <defaultURI
value="activemq:failover:(tcp://localhost:61616,tcp://localhost:61616)"/>

However I have noticed a few issues with the unit tests:

 1. Certain tests fail due to a KeyNotFoundException
 2. The tests run substantially slower

I have identified the KeyNotFoundException to be due to the session state,
which the failover transport maintains.  The KeyNotFoundException was due to
an attempt to remove an invalid session id.  Because the session did not
exist in the dictionary, the KeyNotFoundException was thrown.  However the
caller was checking for a null return value and was not catching this
exception, further up the call stack the exception was being caught and
wrapped in an IOException, thus causing the failover transport to attempt a
re-connect.  The re-connect cycle would then continue forever, and cause the
unit tests to hang.

The reason the session id was invalid was due to a typo in SessionId.cs
where a constructor was using the Value property of a ProducerId rather than
the SessionId property (Patch attached 
http://www.nabble.com/file/p20803252/SessionId.patch SessionId.patch )

I found it much easier to diagnose the issue by adding the NMS trace output
to NUnit, I don't know if that is possible with the existing setup via
configuration files.  If not this patch (
http://www.nabble.com/file/p20803252/UnitTestTracer.patch
UnitTestTracer.patch ) adds the output to the Trace tab, which may be
useful.

Finally, should the SessionState indexer in ConnectionState.cs be changed to
return null if the session does not exist?  Or should the other classes be
catching and ignoring a KeyNotFoundException?
This patch ( http://www.nabble.com/file/p20803252/ConnectionState.patch
ConnectionState.patch ) may be useful for diagnosing when a session id is
missing.  It includes an Assert for when the requested session id does not
exist.


semog wrote:
> 
> I would definitely suggest updating to the latest trunk build.  I haven't
> started my reliable testing/verification as I have been tasked with other
> items.  I would think that the new failover protocol support should help
> in
> this regard.  Prior to that being added, there was no support for
> automatic
> reconnect.  I haven't played with the failover stuff yet, but I would try
> listing the same server twice in the failover list so that the code will
> try
> to reconnect to the same server if it gets bounced.  Don't know if that
> will
> work, but it worth a try.
> 

-- 
View this message in context: http://www.nabble.com/NMS%3A-Small-fix-for-newly-implemented-failover-support-tp20803252p20803252.html
Sent from the ActiveMQ - Dev mailing list archive at Nabble.com.

Re: NMS: Small fix for newly implemented failover support

[semog <e....@gmail.com>]

Hi Daniel,

Thanks for providing this bug report and the supplied patches.  I have
created a new JIRA issue to track this.  Feel free to update it or reference
it here:  https://issues.apache.org/activemq/browse/AMQNET-134 AMQNET-134 .

I applied your patches, so you can sync to the head revision of the trunk
versions to check on them.

Best Regards,
Jim


Daniel Ellis wrote:
> 
> Good work getting the new failover transport in to NMS, which is much
> appreciated and it looks like a substantial piece of work. I have been
> testing the new failover transport, and auto reconnection seems fine by
> adding the same server twice using a connection uri such as:
> 
>   <defaultURI
> value="activemq:failover:(tcp://localhost:61616,tcp://localhost:61616)"/>
> 
> However I have noticed a few issues with the unit tests:
> 
>  1. Certain tests fail due to a KeyNotFoundException
>  2. The tests run substantially slower
> 
> I have identified the KeyNotFoundException to be due to the session state,
> which the failover transport maintains.  The KeyNotFoundException was due
> to an attempt to remove an invalid session id.  Because the session did
> not exist in the dictionary, the KeyNotFoundException was thrown.  However
> the caller was checking for a null return value and was not catching this
> exception, further up the call stack the exception was being caught and
> wrapped in an IOException, thus causing the failover transport to attempt
> a re-connect.  The re-connect cycle would then continue forever, and cause
> the unit tests to hang.
> 
> The reason the session id was invalid was due to a typo in SessionId.cs
> where a constructor was using the Value property of a ProducerId rather
> than the SessionId property (Patch attached 
> http://www.nabble.com/file/p20803252/SessionId.patch SessionId.patch )
> 
> I found it much easier to diagnose the issue by adding the NMS trace
> output to NUnit, I don't know if that is possible with the existing setup
> via configuration files.  If not this patch (
> http://www.nabble.com/file/p20803252/UnitTestTracer.patch
> UnitTestTracer.patch ) adds the output to the Trace tab, which may be
> useful.
> 
> Finally, should the SessionState indexer in ConnectionState.cs be changed
> to return null if the session does not exist?  Or should the other classes
> be catching and ignoring a KeyNotFoundException?
> This patch ( http://www.nabble.com/file/p20803252/ConnectionState.patch
> ConnectionState.patch ) may be useful for diagnosing when a session id is
> missing.  It includes an Assert for when the requested session id does not
> exist.
> 
> 
> semog wrote:
>> 
>> I would definitely suggest updating to the latest trunk build.  I haven't
>> started my reliable testing/verification as I have been tasked with other
>> items.  I would think that the new failover protocol support should help
>> in
>> this regard.  Prior to that being added, there was no support for
>> automatic
>> reconnect.  I haven't played with the failover stuff yet, but I would try
>> listing the same server twice in the failover list so that the code will
>> try
>> to reconnect to the same server if it gets bounced.  Don't know if that
>> will
>> work, but it worth a try.
>> 
> 
> 

-- 
View this message in context: http://www.nabble.com/NMS%3A-Small-fix-for-newly-implemented-failover-support-tp20803252p20964355.html
Sent from the ActiveMQ - Dev mailing list archive at Nabble.com.