Minor Description
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.