[jira] [Commented] (AMQ-6252) HealthView has a variety of weaknesses

["Timothy Bish (JIRA)" <ji...@apache.org>]

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

Timothy Bish commented on AMQ-6252:
-----------------------------------

Since the Broker doesn't know if anyone is even looking at the HealthView bits it wouldn't be optimal to be doing such a heavyweight operation for no good reason which is why there isn't a scheduled task.  The last updated value has some merit although it is hard to settle on what an optimal default for that would be given that everyone would have their own opinion.  

> HealthView has a variety of weaknesses
> --------------------------------------
>
>                 Key: AMQ-6252
>                 URL: https://issues.apache.org/jira/browse/AMQ-6252
>             Project: ActiveMQ
>          Issue Type: Bug
>          Components: Broker
>    Affects Versions: 5.9.1, 5.13.2
>            Reporter: james
>            Priority: Minor
>
> The HealthView utility has a variety of weaknesses.
> * The currentState field should be volatile to ensure correct visibility in a multi-threaded environment
> * the assignment to the currentState field at the end of the healthList method should only happen once the _final_ string has been built.  the current code could expose a caller to a partially built string (since the field is continually re-assigned as the string is built).
> On a separate note, the semantics of this class are non-obvious.  We were monitoring our broker via jmx using the CurrentState attribute for a year or so now and i only just realized that the value is useless.  The value is useless because _it is only updated if you call the healthList() method first_.  The CurrentState attribute is the one someone would most likely monitor using an external automated management product, yet it is not useful by itself.  I would recommend one of the following approaches:
> * at the very least, the current behavior should be _clearly documented_ so that users know they have to call one of the other methods first in order to get a valid CurrentState
> * Alternately, it would be nice if calling the getCurrentState() by itself were sufficient.  this could be accomplished via one of the following approaches:
> ** Have the BrokerService schedule a periodic task to invoke the healthList() method in order to ensure that the CurrentState attribute gets updated.
> ** Track a "last updated" field within the HealthView class and have the getCurrentState() method itself invoke the healthList() method first if the currentState is too old.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)