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

chore(archive): archive and drivers refactor #2761

Merged
merged 22 commits into from
Jul 12, 2024

Conversation

SionoiS
Copy link
Contributor

@SionoiS SionoiS commented Jun 3, 2024

This is a feature branch used to coordinate a big refactor of archive and all drivers to clean/optimize specifically for Store v3.

This work is required before we start measuring performance of Store v3.

@gabrielmer & @Ivansete-status already reviewed all the PRs but if you want to double check go ahead.

What's new are non-PR related commits.

Related PRs;

Closes:

@SionoiS SionoiS changed the title chore(archive) archive and drivers refactor chore(archive): archive and drivers refactor Jun 3, 2024
Copy link

github-actions bot commented Jun 5, 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.

@SionoiS
Copy link
Contributor Author

SionoiS commented Jun 5, 2024

@SionoiS #2756 (comment) done

@SionoiS SionoiS force-pushed the chore--archive-drivers-refactor branch from 254a419 to 9f90434 Compare June 6, 2024 11:37
@SionoiS SionoiS marked this pull request as ready for review June 6, 2024 12:41
Copy link

github-actions bot commented Jun 6, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2761-rln-v1

Built from 3ffa8f5

Copy link

github-actions bot commented Jun 6, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2761-rln-v2

Built from 3ffa8f5

@SionoiS SionoiS marked this pull request as draft June 6, 2024 14:03
@SionoiS
Copy link
Contributor Author

SionoiS commented Jun 6, 2024

I have some more issues to fix before review.

@SionoiS SionoiS marked this pull request as ready for review June 7, 2024 15:39
@SionoiS
Copy link
Contributor Author

SionoiS commented Jun 7, 2024

CI is red but I can't reproduce anything...

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.

Thanks for this PR! This is a very complex task and great work so far!
I'm adding some comments that I hope you find useful.
Let's merge when we revisit all the comments and all CI are green (but the js-waku ones)

@@ -0,0 +1,72 @@
const ContentScriptVersion_6* =
"""
ALTER TABLE IF EXISTS messages_backup RENAME TO messages;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We cannot follow the approach of creating a temporary table because there is normally enough disk space to duplicate the current one. Considering that, we need to perform the database changes directly into the partitioned table, i.e. messages table. That will likely apply the change to the underlying partitions.

Copy link
Contributor Author

@SionoiS SionoiS Jun 11, 2024

Choose a reason for hiding this comment

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

You mean all changes have to be done in-place? Can this be done even for a primary key? Seams complicated with partitions too. I've been looking into it but I can't find a way to do this. We can't even know the name of the partitions in advance.

edit: i did find a way but it doesn't solve the problem of disk space overhead when migrating.

version INTEGER NOT NULL,
timestamp BIGINT NOT NULL,
meta VARCHAR,
id VARCHAR,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to also remove the id field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that we keep the old values for now so that we are still compatible with v2. Would that not work?

tests/all_tests_waku.nim Show resolved Hide resolved
vendor/negentropy Outdated Show resolved Hide resolved
@SionoiS SionoiS added this to the v0.31.0 milestone Jun 11, 2024
@SionoiS SionoiS force-pushed the chore--archive-drivers-refactor branch from 35a4412 to 937c247 Compare June 12, 2024 19:17
@SionoiS
Copy link
Contributor Author

SionoiS commented Jun 12, 2024

Latest error in CI

ERR 2024-06-12 19:30:49.752+00:00 error creating ArchiveDriver               tid=8120 file=postgres_legacy.nim:14 error="postgres health check error: connRes.isErr in query: pool is not live"

I can't reproduce locally.

@Ivansete-status
Copy link
Collaborator

Just for our reference, the following PR is waiting for this #2761 one to be merged: status-im/status-go#5123

Once merged we will need to ping @richard-ramos and then we will test this in status.staging

( cc @SionoiS )

@SionoiS SionoiS force-pushed the chore--archive-drivers-refactor branch from 937c247 to ba56d60 Compare July 2, 2024 15:22
Copy link

github-actions bot commented Jul 2, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2761

Built from 8dc0454

SionoiS and others added 20 commits July 12, 2024 15:23
Co-authored-by: Ivan FB <128452529+Ivansete-status@users.noreply.github.com>
* posgres legacy: stop using the storedAt field
* migration script 6: we still need the id column
  The id column is needed because it contains the message digest
  which is used in store v2, and we need to keep support to
  store v2 for a while
* legacy archive: set target migration version to 6
* waku_node: try to use wakuLegacyArchive if wakuArchive is nil
* node_factory, waku_node: mount legacy and future store simultaneously
  We want the nwaku node to simultaneously support store-v2
  requests and store-v3 requests.
  Only the legacy archive is in charge of archiving messages, and the
  archived information is suitable to fulfill both store-v2 and
  store-v3 needs.
* postgres_driver: adding temporary code until store-v2 is removed

---------

Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>
@Ivansete-status Ivansete-status force-pushed the chore--archive-drivers-refactor branch from 16b692c to 423b4d8 Compare July 12, 2024 13:24
@Ivansete-status Ivansete-status merged commit f54ba10 into master Jul 12, 2024
8 of 10 checks passed
@Ivansete-status Ivansete-status deleted the chore--archive-drivers-refactor branch July 12, 2024 16:19
@Ivansete-status
Copy link
Collaborator

Thank you so much guys for the patience and energy invested in this PR 🙌
I think it looks nice! ( until we find another bug ;P )

@Ivansete-status
Copy link
Collaborator

I've tested that the schema migration works well in a 121GB database ( in wakudev host .)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants