apache/incubator-brooklyn — pull request #257 by aledsage (2014-10-20 09:15:31+00:00)
Title: Fix ServiceFailureDetector

- restore logic to remember last published event, so can take that into account for stabilisation delays
- fix races/timing in tests

Three things I don't like about this code. I haven't tried to tackle these issues in this PR.

1. the need for a mutex in `ServiceFailureDetector.setActualState`. This is new, but I think it was always needed. I just changed the race so that the test failed more often.

/* TODO Really don't want this mutex!
* ServiceStateLogic.setExpectedState() will call into `onEvent(null)`, so could get concurrent calls.
* How to handle that? I don't think `ServiceStateLogic.setExpectedState` should be making the call, but
* presumably that is their to remove a race condition so it is set before method returns. Caller shouldn't
* rely on that though.
* e.g. see `ServiceFailureDetectorTest.testNotifiedOfFailureOnStateOnFire`, where we get two notifications.
*/

2. LOG.warn `Setting TestEntityImpl{id=Ye8VALdQ} on-fire due to problems when expected running, up=false, not-up-indicators: null` appears multiple times. One reason is that `recomputeAfterDelay` will call `onEvent`, which will cause `ServiceStateLogic.ComputeServiceState.computeActualStateWhenExpectedRunning` (i.e. called by super class) to log.warn again.

3. `SERVICE_ON_FIRE_STABILIZATION_DELAY` is a bit confusing. It affects when the on-fire attribute will be set, and is an entirely different configuration setting from `ENTITY_FAILED_STABILIZATION_DELAY` (the latter controls when an "entity-failed" event will be published. That difference is subtle, and there is also no symmetry for setting the entity back to healthy (there is no stabilisation delay available for that).