Skip to content
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 TimePoint and use it in Timeout #1164

Merged
merged 12 commits into from
Aug 21, 2023

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented Aug 1, 2023

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:

  1. I need to measure elapsed time. System.nanoTime is such a tool, but it would have been better to have a tool that exposes types less generic than long, and values that are meaningful on their own (a value returned by System.nanoTime is meaningless on its own).
  2. I need to measure elapsed time from the same instant that a Timeout was started at.

@stIncMale stIncMale self-assigned this Aug 1, 2023
@stIncMale stIncMale force-pushed the JAVA-5076_add_Timer branch 3 times, most recently from 688fc75 to 9551a75 Compare August 2, 2023 21:11
@stIncMale stIncMale marked this pull request as draft August 3, 2023 07:21
@stIncMale stIncMale force-pushed the JAVA-5076_add_Timer branch 3 times, most recently from 5058526 to 6586344 Compare August 4, 2023 05:52

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
JAVA-5076
@stIncMale stIncMale force-pushed the JAVA-5076_add_Timer branch from 6586344 to 807d185 Compare August 4, 2023 06:12
* <hr>
* <sup>(*)</sup> n is positive and odd.
*/
private long elapsedNanos(final long currentNanos) {
Copy link
Member Author

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.

@stIncMale stIncMale marked this pull request as ready for review August 4, 2023 06:28
@stIncMale stIncMale requested review from katcharov and vbabanin August 4, 2023 06:28
It's no longer needed because of mongodb/specifications@31f29fb

JAVA-5076
Copy link
Contributor

@katcharov katcharov left a 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.

@stIncMale stIncMale requested a review from katcharov August 11, 2023 03:10
JAVA-5105
@stIncMale stIncMale changed the title Add Timer and use it in Timeout Add TimePoint and use it in Timeout Aug 15, 2023
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
Copy link
Contributor

@katcharov katcharov left a 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.

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());
Copy link
Member

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());
    }

Copy link
Member Author

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.

Copy link
Member

@vbabanin vbabanin left a 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

@stIncMale stIncMale merged commit c4662b1 into mongodb:master Aug 21, 2023
@stIncMale stIncMale deleted the JAVA-5076_add_Timer branch August 21, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants