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

Propagate Duration from request_timer to the backends #848

Open
luleyleo opened this issue Apr 15, 2020 · 4 comments
Open

Propagate Duration from request_timer to the backends #848

luleyleo opened this issue Apr 15, 2020 · 4 comments
Labels
maintenance cleans up code or docs shell concerns the shell abstraction

Comments

@luleyleo
Copy link
Collaborator

In #847 the api of request_timer was changed to work with Duration instead of Instant.
This was done quickly in order to make the CI work again, by changing request_timer to

pub fn request_timer(&self, deadline: Duration) -> TimerToken {
    self.0.request_timer(instant::Instant::now() + deadline)
}

Which means the back ends were not affected by this.
As @xStrom mentioned on Zulip:

I quickly checked and all platform native functions seem to take duration. Which makes me wonder why druid ended up using Instant at all.

So it would make sens to handle the Duration properly in each backend.

@luleyleo luleyleo added maintenance cleans up code or docs shell concerns the shell abstraction labels Apr 15, 2020
@xStrom
Copy link
Member

xStrom commented Apr 15, 2020

So Duration seems to be the way to go for immediate API calls. One scenario where Instant could be useful is for non-druid druid-shell users who want to request a timer but the request will sit in some queue first and so the timer would be out of sync with Duration but with Instant it would subtract the amount of time it sat in that queue. Not sure this is a feature that druid-shell needs to offer though, because the program that has such a queue could just translate its own Instant to Duration the moment it will invoke druid-shell's timer request method.

Are there any other reasons why Instant makes sense over Duration?

@cmyr
Copy link
Member

cmyr commented Apr 16, 2020

The exact rationale was something like that; that you might compute when you wanted the timer to fire, but have it sit around for a while. I think I went back and forth over which made the most sense, and ultimately just went with Instant; it feels more 'precise' in a mostly theoretical way. I don't mind if this changes.

@xStrom
Copy link
Member

xStrom commented Apr 16, 2020

There are some precision gains by using Instant in a different situation but I think all of that is lost in this specific case by the lack of guarantees that the timer itself has. The timer isn't guaranteed to be accurate at all and can differ from the desired duration/instant by 10ms.

That combined with now having seen a bunch of actual usage where it would be more ergonomical to just use a Duration, I think it might be better to switch to that even for druid-shell.

@cmyr
Copy link
Member

cmyr commented Apr 17, 2020

agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance cleans up code or docs shell concerns the shell abstraction
Projects
None yet
Development

No branches or pull requests

3 participants