-
Notifications
You must be signed in to change notification settings - Fork 221
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
Add wait
operator by porting https://github.com/yoyama/digdag-wait-op
#1520
Conversation
Let me add |
to `wait` operator
wait
operator by porting https://github.com/yoyama/digdag-wait-op
if (!pollIntervalStr.isPresent()) { | ||
return Optional.absent(); | ||
} | ||
pollInterval = Durations.parseDuration(pollIntervalStr.get()); |
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 guess we may make the codes simple by using Optional.transform().
But not strong opinion.
try {
Optional<Duration> pollInterval = config
.getOptional("poll_interval", String.class)
.transform(Durations::parseDuration);
if (pollInterval.isPresent()) {
logger.debug("wait poll_interval: {}", pollInterval.get());
}
return pollInterval;
}
catch (RuntimeException re) {
throw new ConfigException("Invalid configuration", re);
}
assertThat(result.duration, greaterThan(baselineDuration)); | ||
assertThat(result.duration, lessThan( | ||
// Actual wait duration can be longer than the specified 10 seconds for some reason | ||
baselineDuration.plusSeconds((long) (expectedDuration * 1.5)))); |
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 have a bit concern in this condition.
When CI performance is degraded, it may fail unexpectedly.
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.
Agreed. Just tweaked the value since I don't think there is an easy robust way to handle the concern.
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 left some comments.
But LGTM. 👍
Thanks! |
Add `wait` operator by porting https://github.com/yoyama/digdag-wait-op
* Add 'wait' operator * Add test for `wait` operator * Add description about `wait` operator * Refactoring * Add `blocking` and `poll_interval` options to `wait` operator * Small refactoring * Replace nullable value with Optional * Make the key name clearer * Fix typo * Add missing test case * Refactoring * Use a safer expected value * Merge pull request #1520 from treasure-data/wait-oprator Add `wait` operator by porting https://github.com/yoyama/digdag-wait-op Co-authored-by: Mitsunori Komatsu <komamitsu@gmail.com>
The developers and users of Digdag often want to stop a workflow for a specific duration to emulate a long running task.
sh>: sleep 5
may be a workaround, but it's a blocking operator and doesn't fit when we want to emulate long running non-blocking operator task.This pull request ports https://github.com/yoyama/digdag-wait-op with some minor changes to address the above pain point.