DomGarguilo commented on code in PR #6041:
URL: https://github.com/apache/accumulo/pull/6041#discussion_r2673095947
##########
test/src/main/java/org/apache/accumulo/test/util/Wait.java:
##########
@@ -103,14 +106,14 @@ public static void waitFor(final Condition condition,
final long duration, final
final String failMessage) {
final int timeoutFactor = getTimeoutFactor(e -> 1); // default to factor
of 1
- final long scaledDurationNanos = MILLISECONDS.toNanos(duration) *
timeoutFactor;
+ final Duration scaledDuration =
Duration.ofMillis(duration).multipliedBy(timeoutFactor);
Review Comment:
Did some thinking/investigating and I think the `Duration.dividedBy` calls
can safely be replaced, but `multipliedBy()` is safer. If use do regular
primitive `*` then it can silently overflow:
```
jshell> Long.MAX_VALUE * 2
$29 ==> -2
jshell> Duration.ofMillis(Long.MAX_VALUE).multipliedBy(2)
$30 ==> PT5124095576030H25M51.614S
jshell> Duration.ofMillis(Long.MAX_VALUE * 2)
$31 ==> PT-0.002S
```
`Duration` can overflow too but its max is much higher
(`Duration.ofSeconds(Long.MAX_VALUE)`) and it will throw an exception when that
happens instead of silently behaving incorrectly.
IMO, I think its worth it to use the Duration multipliedBy method here to
avoid incorrect behavior. I can easily imagine someone passing Long.MAX_VALUE
to Wait.for() not remembering that it can be scaled internally which could
cause the overflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
