-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BROOKLYN-264: refactor wait-for-provision #266
Conversation
We now mark the internal state as provisioning entirely in MachineLifecycleEffectorTasks, and that is also where we wait for it on stop(). This also allows us to test it as a unit test, rather than needing a jclouds live test.
Transition expectedState = entity().sensors().get(Attributes.SERVICE_STATE_EXPECTED); | ||
|
||
// BROOKLYN-263: see corresponding code in doStop() | ||
if (expectedState != null && (expectedState.getState() == Lifecycle.STOPPING || expectedState.getState() == Lifecycle.STOPPED)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good check, but should it be in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to be in this PR. It relates to concurrent calls to start and stop.
Entities.destroy(app); | ||
return null; | ||
} | ||
}, Asserts.DEFAULT_LONG_TIMEOUT.toMilliseconds(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this too big value?
We now mark the internal state as provisioning entirely in
MachineLifecycleEffectorTasks, and that is also where we wait for it
on stop(). This also allows us to test it as a unit test, rather than
needing a jclouds live test.
@bostko can you please review?
This builds on #211, which is now merged.