-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add TimePoint and use it in Timeout
#1164
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
Conversation
688fc75 to
9551a75
Compare
5058526 to
6586344
Compare
JAVA-5076
6586344 to
807d185
Compare
| * <hr> | ||
| * <sup>(*)</sup> n is positive and odd. | ||
| */ | ||
| private long elapsedNanos(final long currentNanos) { |
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.
This method went to Timer.
It's no longer needed because of mongodb/specifications@31f29fb JAVA-5076
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 am unsure about the approach - I did not review everything in detail, pending the main comment above.
Timer and use it in TimeoutTimePoint and use it in Timeout
JAVA-5105
Instead of having an assertion in `TimePoint` that may detect when a monotonic timer jumps back in time (the OS implementations of monotonic timers are notorious for having bugs, but there is nothing we can do with that), we better allow measuring passing both earlier and later points in `durationSince`. JAVA-5105
JAVA-5105
JAVA-5105
katcharov
left a comment
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.
Just a couple of optional comments.
JAVA-5105
JAVA-5105
JAVA-5105
| long elapsedNanos = elapsedNanos(currentNanos); | ||
| return elapsedNanos < 0 ? 0 : Math.max(0, durationNanos - elapsedNanos); | ||
| long remainingNanos(final TimePoint now) { | ||
| return Math.max(0, durationNanos - now.durationSince(assertNotNull(start)).toNanos()); |
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.
[Optional] According to the javadoc in TimePoint, we can replace TimePoint.now().durationSince(this) with the method elapsed() to avoid passing a parameter down function.
@VisibleForTesting(otherwise = PRIVATE)
long remainingNanos() {
return Math.max(0, durationNanos - assertNotNull(start).elapsed().toNanos());
}
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.
The whole point of exposing remainingNanos for tests is so that tests could pass an arbitrary "now". If this were not needed, remainingNanos would not have been extracted from remaining. With elapsed, using an arbitrary "now" is impossible.
vbabanin
left a comment
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.
LGTM, just one optional comment
This PR introduces internal API needed for JAVA-5076 (see #1166). I separated the change in this PR to make #1166 more focused on business logic.
The internal API introduced in the PR is intended to solve the following problems:
System.nanoTimeis such a tool, but it would have been better to have a tool that exposes types less generic thanlong, and values that are meaningful on their own (a value returned bySystem.nanoTimeis meaningless on its own).Timeoutwas started at.