Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

fix: make timestamps strictly increasing #201

Merged
merged 3 commits into from Jul 16, 2021
Merged

Conversation

Stebalien
Copy link
Member

On Linux, this is almost always the case. Windows, however, doesn't have nanosecond accuracy.

We make the timestamp sequence numbers strictly increasing by returning the last timestamp + 1 where necessary.

On Linux, this is almost always the case. Windows, however, doesn't have
nanosecond accuracy.

We make the timestamp sequence numbers strictly increasing by returning
the last timestamp + 1 where necessary.
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I searched all locally checked out code and all of GitHub, and it looks like TimestampSeq isn't used anywhere. Would removing it be an option?
  2. I find the three atomic operations really hard to reason about. A mutex would be a lot simpler.

peer/record_test.go Outdated Show resolved Hide resolved
peer/record_test.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member Author

I searched all locally checked out code and all of GitHub, and it looks like TimestampSeq isn't used anywhere. Would removing it be an option?

It's used by the NewPeerRecord constructor (here).

I find the three atomic operations really hard to reason about. A mutex would be a lot simpler.

Hm. Yeah, that's probably fine. This is definitely not a hotspot.

Stebalien and others added 2 commits July 16, 2021 14:50
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
@marten-seemann
Copy link
Contributor

It's used by the NewPeerRecord constructor (here).

I meant outside of the repo. There's no real reason to export it. But I'm ok with not changing that (in this PR at least).

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot easier to read, thanks!

@Stebalien
Copy link
Member Author

I meant outside of the repo. There's no real reason to export it. But I'm ok with not changing that (in this PR at least).

Ah. I see... Yeah, probably. But no energy.

@Stebalien Stebalien merged commit ef6e277 into master Jul 16, 2021
@Stebalien Stebalien deleted the fix/monotonic-timestamp branch July 16, 2021 22:02
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants