Generalised Entities.waitFor() #583
Conversation
|
LGTM |
geomacy
left a comment
geomacy
left a comment
There was a problem hiding this comment.
Looks good, have made a couple of suggestions.
Also, for the sake of DRY, if you are introducing this would it not be best to update the implementation of waitForServiceUp to use this new method?
A question - why merge this to 0.9.x? Can/should this not also go into master? In which case maybe merge it there then backport?
| .backoffTo(Duration.ONE_SECOND) | ||
| .rethrowException(); | ||
|
|
||
| if (!repeater.run()) { |
There was a problem hiding this comment.
How about making this capture any exception with runKeepingError so that the throw below could set it as a cause? Also then you can change the message below to refer to whatever the actual error was, rather than assuming it's a timeout.
There was a problem hiding this comment.
Timeout is indicated by the run method returning false, rather than via an exception. Since we set rethrowException, anything thrown by the passed Callable will be rethrown on timeout.
There was a problem hiding this comment.
On the subject of exceptions, I toyed with the idea of throwing TimeoutException instead of IllegalStateException, but am opting for consistency/backwards compatibility with the existing waitForServiceUp methods. (The standard TimeoutException is checked.)
|
|
||
| try { | ||
| Entities.waitFor(entity, applicationIdEqualTo(app.getApplicationId() + "-wrong"), timeout); | ||
| Assert.fail(); |
There was a problem hiding this comment.
Prefer org.apache.brooklyn.test.Asserts#shouldHaveFailedPreviously()
| Assert.fail(); | ||
|
|
||
| } catch (Exception e) { | ||
| // expected timeout |
There was a problem hiding this comment.
Do an assertion on the exception so you're sure it's really what you expected, using one of the Asserts.expectedFailureContains... variants.
alasdairhodge
left a comment
alasdairhodge
left a comment
There was a problem hiding this comment.
Great suggestions, @geomacy; I'll update and push
| return predicate.apply(subject); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Wasn't really sure where to stick this utility method; there are other Callable related methods here, so it seemed as good a place as any.
alasdairhodge
force-pushed
the
generalised-entities-wait-for
branch
from
61f8fc0 to
fc8ec90
Compare
|
@geomacy FYI, I've fixed-up my original commits and force-pushed. I'll should probably close this and issue a fresh PR against |
|
Closing in favour of #584 against |
3 participants
Provides general-purpose blocking wait on any entity predicate. Example: