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.

Sign up for GitHub

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

Jump to bottom

Refactoring timestamps #842

Merged
merged 24 commits into from
Feb 17, 2022
Merged

Refactoring timestamps #842

merged 24 commits into from
Feb 17, 2022

Conversation

s1fr0
Copy link
Contributor

@s1fr0 s1fr0 commented Feb 3, 2022 •
edited
Loading

We introduces a new type Timestamp for all timestamps in waku v2. The type Timestamp is currently set to be an alias for int64 (see utils/time) and all times have now nanosecond resolution.

In particular this PR addresses the following items of #834:

  • The codebase should be refactored to accommodate this change (making sure all the tests pass successfully)
  • Changelog should have an entry briefly explaning this update
  • A migration script should be added to migration scripts directory to handle the schema update in the Message table

@s1fr0 s1fr0 requested a review from staheri14 February 3, 2022 19:34
@s1fr0 s1fr0 marked this pull request as draft February 3, 2022 20:10
@waku-org waku-org deleted a comment from status-im-auto Feb 3, 2022
@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/linux/PR-842#3 🔹 ~16 min 🔹 e01f870 🔹 📦 linux package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/macos/PR-842#3 🔹 ~23 min 🔹 e01f870 🔹 📦 macos package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/windows/PR-842#3 🔹 ~32 min 🔹 e01f870 🔹 📦 windows package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/linux/PR-842#6 🔹 ~16 min 🔹 5371541 🔹 📦 linux package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/macos/PR-842#6 🔹 ~16 min 🔹 5371541 🔹 📦 macos package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/linux/PR-842#5 🔹 ~19 min 🔹 c0249d9 🔹 📦 linux package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/windows/PR-842#4 🔹 ~32 min 🔹 9300fd4 🔹 📦 windows package

@s1fr0 s1fr0 marked this pull request as ready for review February 3, 2022 22:00
@s1fr0 s1fr0 linked an issue Feb 3, 2022 that may be closed by this pull request
Changing timestamp type to int64 #834
Closed
4 tasks
@s1fr0 s1fr0 marked this pull request as draft February 3, 2022 22:16
@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/windows/PR-842#8 🔹 ~30 min 🔹 5371541 🔹 📦 windows package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/linux/PR-842#8 🔹 ~16 min 🔹 00adea6 🔹 📦 linux package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/macos/PR-842#8 🔹 ~16 min 🔹 00adea6 🔹 📦 macos package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/windows/PR-842#10 🔹 ~33 min 🔹 00adea6 🔹 📦 windows package

@arnetheduck
Copy link
Contributor

As a general recommendation, going with nanoseconds-since-epoch for timestamps is the "standard" these days, and has few downsides (except perhaps that you need to copypaste a bigger number).

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/linux/PR-842#29 🔹 ~16 min 🔹 a21de43 🔹 📦 linux package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/macos/PR-842#29 🔹 ~23 min 🔹 a21de43 🔹 📦 macos package

staheri14
staheri14 reviewed Feb 9, 2022
View reviewed changes
waku/v2/utils/time.nim
var ms = Timestamp(timeInSeconds*1000)
return ms

proc column_timestamp*(a1: ptr sqlite3_stmt, iCol: cint): int64 =
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for defining this proc, though I think it is also ok to use the sqlite3_column_int64 directly in the code. I'd like to hear @jm-clius opinion as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here a related comment to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I cannot see any issue with having a separate proc column_timestamp

@staheri14
Copy link
Contributor

staheri14 commented Feb 9, 2022 •
edited
Loading

  1. Depending on a discussion in Refactoring timestamps vacp2p/rfc#483, we likely need to bump the protocol-id/Codec to indicate us breaking backwards compatibility on the protocol.

Good point, agree!

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/windows/PR-842#31 🔹 ~34 min 🔹 a21de43 🔹 📦 windows package

@staheri14
Copy link
Contributor

@s1fr0 The PR looks good to me! just awaiting the result of the decision about the protocol id #842 (comment) (after that I'll approve :) ).

@jm-clius jm-clius mentioned this pull request Feb 11, 2022
Merged
@oskarth oskarth added the blocked This issue is blocked by some other work label Feb 17, 2022
@oskarth
Copy link
Contributor

oskarth commented Feb 17, 2022

Temporarily blocked, see vacp2p/rfc#483 (comment)

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/linux/PR-842#32 🔹 ~17 min 🔹 fe09c2a 🔹 📦 linux package

@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/macos/PR-842#32 🔹 ~22 min 🔹 fe09c2a 🔹 📦 macos package

@s1fr0 s1fr0 removed the blocked This issue is blocked by some other work label Feb 17, 2022
@status-im-auto
Copy link
Collaborator

✔️ nim-waku/prs/windows/PR-842#36 🔹 ~35 min 🔹 fe09c2a 🔹 📦 windows package

@s1fr0 s1fr0 merged commit 21cac6d into master Feb 17, 2022
@s1fr0 s1fr0 deleted the int64-timestamps branch February 17, 2022 15:00
@D4nte
Copy link
Contributor

D4nte commented Feb 17, 2022

I can't see the change of the waku store codec string from beta3 to beta4 in this PR, is this expected? @s1fr0

@D4nte D4nte mentioned this pull request Feb 17, 2022
Closed
@s1fr0 s1fr0 restored the int64-timestamps branch February 17, 2022 23:36
This was referenced Feb 17, 2022
Closed
Merged
@s1fr0
Copy link
Contributor Author

s1fr0 commented Feb 17, 2022

@D4nte thanks a lot for pointing it out. No, it is not. I created a new PR #855 for this. You may review it, if you can.

@richard-ramos richard-ramos mentioned this pull request Feb 18, 2022
Merged
This was referenced Feb 22, 2022
Merged
Merged
@richard-ramos richard-ramos mentioned this pull request Jun 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staheri14 staheri14 staheri14 left review comments

@jm-clius jm-clius jm-clius approved these changes

@kaiserd kaiserd Awaiting requested review from kaiserd

@oskarth oskarth Awaiting requested review from oskarth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changing timestamp type to int64
8 participants
@s1fr0 @status-im-auto @arnetheduck @jm-clius @staheri14 @richard-ramos @D4nte @oskarth