activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paul Gale (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (AMQ-4674) ActiveMQ 5.x does not support the notion of a grace-period for heart beats as supported by the STOMP protocol
Date Tue, 20 Aug 2013 21:25:51 GMT

    [ https://issues.apache.org/jira/browse/AMQ-4674?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13745438#comment-13745438
] 

Paul Gale edited comment on AMQ-4674 at 8/20/13 9:24 PM:
---------------------------------------------------------

I have no idea how to write unit tests for ActiveMQ and this section of code in particular.


Unfortunately I could not find any unit tests for this feature which is surprising given that
it was added just the other day. How was it tested? It's a tad galling to be asked for some
tests now.

I don't mind modifying existing tests though. However, my personal experience with the unit
test codebase is that they're rather flaky; they almost never pass whenever I've tried to
run them which doesn't exactly entice me into wanting to try now.

Nonetheless, from a quick analysis of the code it would appear that the offending code is
in activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java
at line 929:
\\
{code:java|borderStyle=solid}
    hbReadInterval = (long) (Long.parseLong(keepAliveOpts[0]) * hbGracePeriodMultiplier);
// Wrong!
{code}

should be:
\\
{code:java|borderStyle=solid}
    hbReadInterval = (long) Long.parseLong(keepAliveOpts[0]); //  Honor the client's read
interval
{code}

where keepAliveOpts[0] is the client specified heartbeat read-interval.

When the inactivity monitor's read check time is calculated it's done correctly:
\\
{code:java|borderStyle=solid}
StompInactivityMonitor monitor = this.stompTransport.getInactivityMonitor();
monitor.setReadCheckTime((long) (hbReadInterval * hbGracePeriodMultiplier));  // Correct
monitor.setInitialDelayTime(Math.min(hbReadInterval, hbWriteInterval));
monitor.setWriteCheckTime(hbWriteInterval);
monitor.startMonitoring();
{code}

{noformat}
Setup:    keepAliveOpts[0] = 5000, hbGracePeriodMultiplier = 1.5

Expected: hbReadInterval == 5000, monitor.getReadCheckTime() == 7500
Actual:   hbReadInterval == 7500, monitor.getReadCheckTime() == 11250
{noformat}

                
      was (Author: paulgale):
    I have no idea how to write unit tests for ActiveMQ and this section of code in particular.


Unfortunately I could not find any unit tests for this feature which is surprising given that
it was added just the other day. How was it tested? It's a tad galling to be asked for some
tests now.

I don't mind modifying existing tests though. However, my personal experience with the unit
test codebase is that they're rather flaky; they almost never pass whenever I've tried to
run them which doesn't exactly entice me into wanting to try now.

Nonetheless, from a quick analysis of the code it would appear that the offending code is
in activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java
at line 929:

{code:java|borderStyle=solid}
    hbReadInterval = (long) (Long.parseLong(keepAliveOpts[0]) * hbGracePeriodMultiplier);
// Wrong!
{code}

should be:


{code:java|borderStyle=solid}
    hbReadInterval = (long) Long.parseLong(keepAliveOpts[0]); //  Honor the client's read
interval
{code}

where keepAliveOpts[0] is the client specified heartbeat read-interval.

When the inactivity monitor's read check time is calculated it's done correctly:


{code:java|borderStyle=solid}
StompInactivityMonitor monitor = this.stompTransport.getInactivityMonitor();
monitor.setReadCheckTime((long) (hbReadInterval * hbGracePeriodMultiplier));  // Correct
monitor.setInitialDelayTime(Math.min(hbReadInterval, hbWriteInterval));
monitor.setWriteCheckTime(hbWriteInterval);
monitor.startMonitoring();
{code}

Setup:    keepAliveOpts[0] = 5000, hbGracePeriodMultiplier = 1.5

Expected: hbReadInterval == 5000, monitor.getReadCheckTime() == 7500
Actual:   hbReadInterval == 7500, monitor.getReadCheckTime() == 11250


                  
> ActiveMQ 5.x does not support the notion of a grace-period for heart beats as supported
by the STOMP protocol
> -------------------------------------------------------------------------------------------------------------
>
>                 Key: AMQ-4674
>                 URL: https://issues.apache.org/jira/browse/AMQ-4674
>             Project: ActiveMQ
>          Issue Type: Bug
>    Affects Versions: 5.8.0
>            Reporter: Paul Gale
>            Assignee: Timothy Bish
>              Labels: easyfix
>             Fix For: 5.9.0
>
>
> Regarding the configuration of heart beating the STOMP protocol spec states:
>     "- because of timing inaccuracies, the receiver SHOULD be tolerant and take into
account an error margin"
> However, it appears that ActiveMQ 5.x is not tolerant of any error margin. 
> Despite the fact that the spec says SHOULD rather than MUST it would make the implementation
of STOMP clients easier if the error margin was published.
> As the broker aggressively enforces the heart beat timeouts false failover attempts can
result.
> Apparently Apollo supports an error margin of 1.5x the configured heart beat. If it could
be made configurable that would be even better! 

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message