Port Clock functions to use Duration class
#19229
Conversation
erikjohnston
force-pushed
the
erikj/duration
branch
from
a386db9 to
112a34d
Compare
erikjohnston
force-pushed
the
erikj/duration
branch
from
112a34d to
901f1fe
Compare
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
Really nice readability improvements across the board!
I went through each of the conversions and they looked to be correct. Just a few things below.
|
|
||
| def as_millis(self) -> int: | ||
| """Returns the duration in milliseconds.""" | ||
| return int(self / _ONE_MILLISECOND) |
There was a problem hiding this comment.
I assume sub-millisecond accuracy doesn't really matter in our case and thus we don't need to do the same for as_millis? (Edit: well, we found one case where it might).
We should note that milliseconds is the smallest time unit this class can work with in the Duration docstring.
|
|
||
| # How often we expect remote servers to resend us presence. | ||
| FEDERATION_TIMEOUT = 60 * 1000 | ||
| FEDERATION_TIMEOUT = Duration(seconds=60) |
There was a problem hiding this comment.
We should be consistent regarding Duration(minutes=1) vs. Duration(seconds=60). Personally I prefer the former.
| if hs.config.worker.run_background_tasks: | ||
| self.clock.looping_call( | ||
| self.cull_expired_threepid_validation_tokens, THIRTY_MINUTES_IN_MS | ||
| self.cull_expired_threepid_validation_tokens, THIRTY_MINUTES |
There was a problem hiding this comment.
We can potentially just inline Duration(minutes=30) here now? I suspect the only reason for the global variable was because 30 * 60 * 1000 was more awful to inline.
| # Precision of the scheduler, evaluation of tasks to run will only happen | ||
| # every `SCHEDULE_INTERVAL_MS` ms | ||
| SCHEDULE_INTERVAL_MS = 1 * 60 * 1000 # 1mn | ||
| # every `SCHEDULE_INTERVAL` ms |
There was a problem hiding this comment.
| # every `SCHEDULE_INTERVAL` ms | |
| # every `SCHEDULE_INTERVAL` |
| def _scheduler(x: Callable[[], object]) -> IDelayedCall: | ||
| return clock.call_later( | ||
| _EPSILON, | ||
| Duration(seconds=_EPSILON), |
There was a problem hiding this comment.
We're losing some accuracy here (though I'm not sure how much it matters in this case).
The smallest unit datetime.timedelta deals with is microseconds. _EPSILON is 0.01 microseconds (10 nanoseconds), so Duration(seconds=_EPSILON).as_millis() will result in 1.
I think it'd also be cleaner to write either:
_EPSILON = Duration(milliseconds=1)or inline it:
return clock.call_later(
Duration(milliseconds=1),
x,
)
| let duration_module = py.import("synapse.util.duration")?; | ||
|
|
||
| let kwargs = [("milliseconds", eviction_interval)].into_py_dict(py)?; | ||
| let eviction_duration = duration_module.call_method("Duration", (), Some(&kwargs))?; |
There was a problem hiding this comment.
Hmm, this is a little horrible. Is it possible to make a Rust struct that wraps synapse.util.duration that we can easily work with?
This changes the arguments in clock functions to be
Durationand converts call sites and constants intoDuration. There are still some more functions around that should be converted (e.g.timeout_deferred), but we leave that to another PR.We also changes
.as_secs()to return a float, as the rounding broke things subtly. The only reason to keep it (its the same astimedelta.total_seconds()) is for symmetry withas_millis().Follows on from #19223
Reviewable commit-by-commit.