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

feat: archive update for store v3 #2451

Merged
merged 9 commits into from
Mar 12, 2024
Merged

feat: archive update for store v3 #2451

merged 9 commits into from
Mar 12, 2024

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Feb 19, 2024 •
edited
Loading

Description

I tried a bunch of things that didn't work. In the end I simply added hashed to requests and responses. This approach will require a good clean-up after store v2 is removed.

I simplified how message are validated since we never exposed the validation even if the intent seamed to be to make it configurable. Do we want fully configurable validation? What kind of messages do we reject exactly?

There are also couple of comments that require reviewers input.

Changes

  • refactored WakuArchive
  • archive cursor new use timestamp and msg hash
  • simplified msg validation
  • compatibility with Store v2
  • update drivers
  • update tests

Tracking #2425

@SionoiS SionoiS self-assigned this Feb 20, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2451

Built from f03ee1c

@SionoiS SionoiS marked this pull request as ready for review March 5, 2024 16:06
@SionoiS
Copy link
Contributor Author

SionoiS commented Mar 6, 2024 •
edited
Loading

Will have to wait for #2506, resolve and then review.

@SionoiS SionoiS marked this pull request as draft March 6, 2024 13:18
Ivansete-status
Ivansete-status requested changes Mar 6, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Overall it looks great! I'm just adding a few points that I think we need to double-check

tests/waku_archive/test_waku_archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/node/waku_node.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/driver.nim Outdated Show resolved Hide resolved
waku/waku_archive/driver/postgres_driver/postgres_driver.nim Outdated Show resolved Hide resolved
waku/waku_archive/driver/sqlite_driver/sqlite_driver.nim Outdated Show resolved Hide resolved
@SionoiS SionoiS marked this pull request as ready for review March 7, 2024 15:50
Ivansete-status
Ivansete-status approved these changes Mar 7, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the patience!

waku/waku_archive/driver/postgres_driver/postgres_driver.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Show resolved Hide resolved
waku/waku_archive/driver/postgres_driver/postgres_driver.nim Outdated Show resolved Hide resolved
waku/waku_archive/driver/sqlite_driver/queries.nim Outdated Show resolved Hide resolved
gabrielmer
gabrielmer approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

Amazing work! 🔥 🔥 🔥
Thanks so much!

waku/waku_archive/driver/sqlite_driver/queries.nim Outdated Show resolved Hide resolved
tests/waku_archive/test_driver_postgres_query.nim Outdated Show resolved Hide resolved
jm-clius
jm-clius approved these changes Mar 8, 2024
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.

Thanks - this was quite a detailed effort. I've added a few comments, but approving since none are blocking. One general comment: there do seem to be quite a few changes unrelated to what is exactly needed for archive queries to support v3. I get the impulse, as there's clearly a lot that can be improved, but may be good to separate into separate PRs as much as possible in future to derisk and make reviewing a bit easier. :)

waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/archive.nim Outdated Show resolved Hide resolved
waku/waku_archive/driver/postgres_driver/postgres_driver.nim
@@ -257,6 +266,12 @@ proc getMessagesArbitraryQuery(s: PostgresDriver,
for t in contentTopic:
args.add(t)

if hexHashes.len > 0:
let cstmt = "messageHash IN (" & "?".repeat(hexHashes.len).join(",") & ")"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an index built on this column? Would it be necessary for querying on message hash to be efficient? Perhaps we'll need a benchmark on a rather large DB (future work).

Copy link
Contributor Author

@SionoiS SionoiS Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can't find any indexing. It would be good to take a good look at the Db schema once Store v2 is remove. I feel we could get better perfs.

Yes testing will tell us what to expect performance wise. We may have to use LMDB in the end (I hope not).

@SionoiS
Copy link
Contributor Author

SionoiS commented Mar 8, 2024

there do seem to be quite a few changes unrelated to what is exactly needed for archive queries to support v3

Yes, some part of the code was not up to our current standards.

I get the impulse, as there's clearly a lot that can be improved, but may be good to separate into separate PRs as much as possible in future to derisk and make reviewing a bit easier. :)

Guilty :P I need to get better at this.

@SionoiS SionoiS merged commit 505479b into master Mar 12, 2024
9 of 10 checks passed
@SionoiS SionoiS deleted the feat--archive-update branch March 12, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@gabrielmer gabrielmer gabrielmer approved these changes

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

Assignees

@SionoiS SionoiS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SionoiS @jm-clius @gabrielmer @Ivansete-status