search

datafuselabs / openraft

rust raft with improvements
Apache License 2.0
1.36k stars 153 forks source link

Timekeeping in standard units #1178

Open schreter opened 2 months ago

schreter commented 2 months ago

Currently, openraft uses various time units. For example, a lot of timeouts and time intervals is stored in milliseconds. However, this makes it sometime cumbersome and/or confusing to handle time. Aside from that, it requires additional multiplications/divisions.

My suggestion is to change all time fields to standard types Instant and Duration and where needed (e.g., on API level) provide old methods which recompute internally.

Similarly, storing "time since" is problematic (I've seen it somewhere, not sure where), since it doesn't say when it was computed. In such cases, storing an Instant of the event would be better. Then, at now(), it's easily possible to compute the duration by a simple subtraction.

github-actions[bot] commented 2 months ago

👋 Thanks for opening this issue!

Get help or engage by:

drmingdrmer commented 2 months ago

I agree. I'll update the API to use Instant of Duration if possible.

There is one time since AFAIK: https://github.com/datafuselabs/openraft/blob/9dbcd4c0b6723fd4ac5d38ed032d8e656d0abc8d/openraft/src/metrics/raft_metrics.rs#L70

This is because the RaftMetrics must be serde thus it can not store an Instant directly in it. An options is to convert this field to a time string. But IMHO a relative interval since the last time the metrics is reported would be easier to use, except its inaccuracy.

What do you think about adding the current wall clock time into the RaftMetrics?

schreter commented 1 month ago

This is because the RaftMetrics must be serde thus it can not store an Instant directly in it.

Hm... Isn't it possible to store it as an Instant and simply serialize differently (as an UTC DateTime or similar)? Then, on the remote node, it would deserialize from UTC back to Instant, even if the implementation there is different.

I'm no serde expert, but there must be a way to explicitly describe serialization/deserialization of fields (at least, there is the possibility of custom serialization, which could likely be implemented on a simple Newtype wrapping the Instant).