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

History response pagination using sender-generated timestamp #657

Merged
merged 17 commits into from
Jul 7, 2021
Merged

History response pagination using sender-generated timestamp #657

merged 17 commits into from
Jul 7, 2021

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Jul 2, 2021 •
edited
Loading

Closes vacp2p/rfc#219 and partial progress towards vacp2p/rfc#406
Paging based on sender timestamp is more conventional, and in this PR the role of receiver timestamp in the pagination is entirely replaced by the sender timestamp.

@status-im-auto
Copy link
Collaborator

status-im-auto commented Jul 2, 2021 •
edited
Loading

Jenkins Builds

Click to see older builds (36)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f97d543 #1 2021-07-02 23:33:33 ~17 min macos 📦bin
❌ f97d543 #1 2021-07-02 23:42:52 ~26 min linux 📄log
✔️ f97d543 #1 2021-07-03 00:28:36 ~1 hr 12 min windows 📦exe
❌ d1ff6ca #3 2021-07-02 23:33:42 ~14 min macos 📄log
❌ d1ff6ca #3 2021-07-02 23:39:59 ~20 min linux 📄log
✔️ d1ff6ca #3 2021-07-03 00:18:03 ~58 min windows 📦exe
✔️ 9f7b503 #2 2021-07-02 23:33:51 ~15 min macos 📦bin
❌ 9f7b503 #2 2021-07-02 23:53:37 ~35 min linux 📄log
✔️ 9f7b503 #2 2021-07-03 00:18:30 ~1 hr 0 min windows 📦exe
✔️ 8f7475e #4 2021-07-02 23:39:36 ~10 min macos 📦bin
❌ 8f7475e #4 2021-07-02 23:57:20 ~28 min linux 📄log
❌ 8f7475e #5 2021-07-03 00:03:43 ~45 sec linux 📄log
❌ 8f7475e #6 2021-07-03 00:12:47 ~53 sec linux 📄log
✔️ 8f7475e #4 2021-07-03 00:26:55 ~57 min windows 📦exe
✔️ 8f7475e #7 2021-07-03 00:30:01 ~13 min linux 📦bin
✔️ 912ca42 #5 2021-07-05 18:38:34 ~11 min macos 📦bin
✔️ 912ca42 #8 2021-07-05 18:41:25 ~13 min linux 📦bin
✔️ 912ca42 #5 2021-07-05 19:01:44 ~34 min windows 📦exe
❌ a4fd28e #6 2021-07-06 22:47:54 ~5 min macos 📄log
❌ a4fd28e #9 2021-07-06 22:54:39 ~12 min linux 📄log
❌ a4fd28e #6 2021-07-06 23:08:37 ~26 min windows 📄log
❌ 85a7c43 #8 2021-07-06 22:51:58 ~6 min macos 📄log
❌ 85a7c43 #11 2021-07-06 23:04:19 ~18 min linux 📄log
❌ 85a7c43 #8 2021-07-06 23:26:09 ~40 min windows 📄log
❌ 74b46e9 #7 2021-07-06 22:53:21 ~8 min macos 📄log
❌ 74b46e9 #10 2021-07-06 22:56:25 ~11 min linux 📄log
❌ 74b46e9 #7 2021-07-06 23:28:05 ~43 min windows 📄log
❌ 4612072 #9 2021-07-06 22:54:40 ~7 min macos 📄log
❌ 4612072 #12 2021-07-06 22:55:27 ~8 min linux 📄log
❌ 4612072 #9 2021-07-06 23:49:29 ~1 hr 2 min windows 📄log
❌ 5cfbf7c #10 2021-07-06 22:55:56 ~6 min macos 📄log
❌ 5cfbf7c #13 2021-07-06 22:56:43 ~7 min linux 📄log
❌ 5cfbf7c #10 2021-07-06 23:51:52 ~1 hr 2 min windows 📄log
❌ 6af6909 #11 2021-07-06 23:15:44 ~7 min macos 📄log
❌ 6af6909 #14 2021-07-06 23:16:44 ~8 min linux 📄log
❌ 6af6909 #11 2021-07-07 00:08:52 ~1 hr 0 min windows 📄log
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 8dfd22f #12 2021-07-06 23:21:54 ~12 min macos 📦bin
✔️ 8dfd22f #15 2021-07-06 23:32:59 ~23 min linux 📦bin
✔️ 8dfd22f #12 2021-07-07 00:19:28 ~1 hr 9 min windows 📦exe
✔️ 2b1ea81 #13 2021-07-07 17:08:05 ~11 min macos 📦bin
✔️ 2b1ea81 #16 2021-07-07 17:11:15 ~14 min linux 📦bin
✔️ 2b1ea81 #13 2021-07-07 17:32:49 ~35 min windows 📦exe

@staheri14 staheri14 self-assigned this Jul 2, 2021
@staheri14 staheri14 mentioned this pull request Jul 2, 2021
Merged
@staheri14 staheri14 requested review from oskarth, jm-clius and EbubeUd July 3, 2021 00:12
@staheri14
Copy link
Contributor Author

@jakubgs the Jenkins log does not give much info to be able to track the failure, what are your thoughts? Thanks!

@staheri14
Copy link
Contributor Author

@jakubgs I restarted the runs and errors are gone!

oskarth
oskarth reviewed Jul 5, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
waku/v2/node/storage/message/waku_message_store.nim Outdated Show resolved Hide resolved
jm-clius
jm-clius reviewed Jul 5, 2021
View reviewed changes
tests/v2/test_wakunode.nim Outdated Show resolved Hide resolved
jm-clius
jm-clius reviewed Jul 5, 2021
View reviewed changes
waku/v2/node/storage/migration/migrations_scripts/message/00003_deleteReceiverTimeStamp.up.sql Outdated
) WITHOUT ROWID;


INSERT INTO Message SELECT id, contentTopic, pubsubTopic, payload, version, senderTimestamp FROM Message_backup;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering for interest's sake what the benefit is of doing it this way (creating a new table based on the previous) vs just deleting the column (ALTER TABLE...DROP COLUMN)? This will still involve making a backup copy, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, unfortunately, SQlite does not support the ALTER TABLE command

@staheri14
Copy link
Contributor Author

staheri14 commented Jul 6, 2021 •
edited
Loading

Updated the PR: @oskarth @jm-clius
In the current update, the receiver timestamp remains in the Index struct and gets transferred as part of the paging info protobuf, however, in the nim-waku implementation, it has no effect on the sorting of waku messages during the pagination. Sorting is only based on the message digest and the sender timestamp.

@staheri14 staheri14 requested a review from oskarth July 6, 2021 23:16
@staheri14 staheri14 requested a review from jm-clius July 6, 2021 23:16
oskarth
oskarth approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@oskarth oskarth left a comment

Choose a reason for hiding this comment

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

LGTM

jm-clius
jm-clius approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Nice!

@staheri14 staheri14 merged commit cf7b8fa into master Jul 7, 2021
@staheri14 staheri14 deleted the pagination-with-sender-timestamp branch July 7, 2021 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@oskarth oskarth oskarth approved these changes

@EbubeUd EbubeUd Awaiting requested review from EbubeUd

Assignees

@staheri14 staheri14

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

13/WAKU2-STORE: History response pagination using sender-generated timestamp
4 participants
@staheri14 @status-im-auto @oskarth @jm-clius