Added start latch to BasicStartable and StartableApplication#37
Added start latch to BasicStartable and StartableApplication#37asfgit merged 5 commits intoapache:masterfrom
Conversation
|
|
||
| // Opportunity to block startup until other dependent components are available | ||
| Object val = config().get(START_LATCH); | ||
| if (val != null) log.debug("{} finished waiting for start-latch; continuing...", this, val); |
There was a problem hiding this comment.
There's no matching {} for val in the message.
|
Two trivial comments, otherwise good to merge. |
| // default app logic; easily overridable by adding a different enricher with the same tag | ||
| ServiceStateLogic.newEnricherFromChildren().checkChildrenAndMembers().addTo(this); | ||
| ServiceStateLogic.ServiceNotUpLogic.updateNotUpIndicator(this, Attributes.SERVICE_STATE_ACTUAL, "Application created but not yet started, at "+Time.makeDateString()); | ||
| ServiceStateLogic.ServiceNotUpLogic.updateNotUpIndicator(this, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.CREATED); |
There was a problem hiding this comment.
Why change these, out of interest? They are now inconsistent with the calls in stop() (which pass "Application stopping" etc).
There was a problem hiding this comment.
Because the not-up indicator is checking a sensor against its value, and the Attributes.SERVICE_STATE_ACTUAL is of type Lifecycle. I'll also change stop() ten...
There was a problem hiding this comment.
The attribute is passed just for the name, its type is not used in the not-up map. Human readable values are preferred as that's its primary target - troubleshooting by users.
grkvlt
force-pushed
the
start-latch
branch
2 times, most recently
from
50e5424 to
2017bac
Compare
|
Updated and squashed commits |
|
Added another commit to fix issue with Clocker where |
| }}); | ||
|
|
||
| boolean skipInstall = locationInstalled.or(entityInstalled).or(false); | ||
| if (!skipInstall) { |
There was a problem hiding this comment.
Just to state the cases we need to test or to have automated test:
- Install multiple apps on a BYON location
- Perform a restart on a BYON location
- Perform a restart with machineStop: NEVER/(or false)
There was a problem hiding this comment.
What I really want is a stage that is always executed on entity startup, maybe not preInstall() but prepare() that doesn't do anything on the VM, but sets up the entity instance. This is where the setting of expandedInstallDir and so on would happen. I'll see if it's easy to make that change.
|
@bostko I think this (and apache/brooklyn-library#14) will do what you want. The |
grkvlt
force-pushed
the
start-latch
branch
2 times, most recently
from
37d72d4 to
f0d0c68
Compare
|
@grkvlt Can you rebase this please? |
|
@sjcorbett done |
|
Tests pass locally. |
|
@sjcorbett have to confirmed that downstream entities work with this PR and with no changes? NB there are a lot of related changes in but (hopefully) these are cleaning it up, not requirements. (if this PR breaks backwards compatibility then we have to document that very clearly!) |
| doStart(locationsToUse); | ||
| postStart(locationsToUse); | ||
|
|
||
| ServiceStateLogic.ServiceNotUpLogic.clearNotUpIndicator(this, Attributes.SERVICE_STATE_ACTUAL); |
There was a problem hiding this comment.
feels odd to do this after start, we might be clearing important not-up indications which happened during doStart etc. (ideally we probably shouldn't have to do it at all.)
not a big deal as logic has not been perfect but maybe worth a comment in the code
There was a problem hiding this comment.
i guess this is how we do it elsewhere, we basically assume if start failed without errors then it should normally be up, and we'll get told subsequently + periodically if that isn't the case.
in which case it makes sense to me to put this after, else we might catch a rogue not-up indicator from before the start which isn't reset in time.
(but still worth a comment in the code!)
|
don't like seeing expansion of root lifeycle methods ( nice @grkvlt |
|
@ahgittin I ran the library/nosql+webapp integration tests. All nosql tests passed. The webapp tests that failed did not do so because of this change. As I understood it library-#14 shifts entity's previous behaviour from |
|
Thanks @sjcorbett . I haven't noticed any problems either. |
|
@sjcorbett correct apache/brooklyn-library#14 needs merged to fix Clocker |
Update default docker image id to latest build
7 participants
Allows groups of components and applications to have their startup sequence controlled more easily. This includes only setting
service.isUptotrueon anAbstractApplicationonce it, and its children, have started. This is the same semantics as other entities, and allows a more consistent blueprint style.Also reverts d7c3759 which prevents Clocker from re-using images.