Add waitFor method in core. Relocate UtilWaitThread.sleep #4002
Conversation
| * @return return a fluent-style object to configure other optional parameters. | ||
| */ | ||
| public WaitFor upTo(final long duration, TimeUnit units) { | ||
| if (duration <= 0) { |
There was a problem hiding this comment.
Why not use Preconditions.checkArgument() if the inputs are invalid?
There was a problem hiding this comment.
Rather than fail I opted to fall back to the default value.
There was a problem hiding this comment.
Changed to use Preconditions.checkArgument() in 775897d
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
So, my observation right now is that you took the current Wait class, copied it, stripped out the time scaling, added other features like a builder and debug logging, and then didn't use any of it. Also, moved sleep from one class to another. This PR alone doesn't really justify any changes, because it's basically just an unneeded move, and some features like the builder seem overly complex for what's could be a very simple tool. But, you said you were working to set the stage to make use of it later, so I can't really have a complete thought on the value / complexity tradeoff until i see how it might be used.
Instead of doing it this way, though, I would probably try to consolidate, so that the the current Wait and WaitFor are using the same base code, and make it so the timeout stuff can be passed as a function, rather than included in the base Wait class. I would also replace "WaitFor.sleep(long)" "Wait.millis(long)", so the grammar works better: "wait for sleep X [ambiguous time units (read docs to see that it's millis)]" is less good when read aloud than what can be read as "wait X milliseconds".
For me, I would start by moving Wait into core.util and adding sleep in one PR... the first PR would focus on making the interface to pass the timeout stuff clear. Then, once that's merged and there's no duplication between the two classes any more, I'd add slight improvements/features, only as needed, along with the subsequent PRs that require those improvements and immediately make use of them. I would probably not do a builder at all, because that complexity isn't needed here. We can just have "Wait.condition()" or "Wait.time" methods that are very simple and reusable, rather than creating a builder for a "Waiter" object that can be configured in complex ways. You can add hooks for behavior changes, like logging/exception handling, like passing an "onStart" consumer as a parameter or an "onError" consumer, like what I did for the current Wait class.
In conclusion:
- The refactor and the addition of new features can be done separately, with the new features added only when needed,
- I think it could benefit from some more code reuse with the existing Wait class, and some better method names, and
- I think the builder stuff is overkill, but
- Overall, I think I agree it would be good to move this into the core module in some way, so that other code can make use of similar patterns.
| Thread.sleep(millis); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| LOG.error("{}", e.getMessage(), e); |
There was a problem hiding this comment.
could improve this message a bit while we're here
| LOG.error("{}", e.getMessage(), e); | |
| LOG.error("sleep interrupted", e); |
|
It is currently unused because I was waiting for some version to get approved so that I could move forward. There are other PRs that I have open where it was suggested that I use Wait.waitFor - which seemed like an improvement, so I started down this path instead. I didn't think that the other PR changes needed to be bundled into this issue. |
3 participants
The test module has a convenient Wait.waitFor that could be used generally, however, that code allows for a scaling factor to allow for variations in test environments. This PR replicates that functionality for general use outside of the test module.
Rather that static methods, this uses fluent-style builder that can be concise while providing customization. It also has a signature different from the test waitFor methods to avoid confusion which wait is being used.
This also relocates the sleep method from UtilWaitThread and deletes that class. The original intent of UtilWaitThread class was to hold guava sleepUninterruptibly code that had been marked as beta at one time. Using the guava provided sleepUninterruptibly was completed in a previous PR. The relocated sleep also reestablishes the interrupt flag that can be used by callers to determine if in interrupt occurred.