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

Timekeeping in standard units #1178

Open
schreter opened this issue Jul 18, 2024 · 3 comments
Open

Timekeeping in standard units #1178

schreter opened this issue Jul 18, 2024 · 3 comments

Comments

@schreter
Copy link
Collaborator

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.

Copy link

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@drmingdrmer
Copy link
Member

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
Copy link
Collaborator Author

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).

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

No branches or pull requests

2 participants