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

refactor: new proc to foster different size retention policy implementations #2463

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

Ivansete-status
Copy link
Collaborator

Description

The new proc, decreaseDatabaseSize, will have different implementations per driver. For example, in future commits, we will implement a size retention policy thanks to partitions management, in Postgres.

Changes

  • New method decreaseDatabaseSize in driver.nim.
  • Split the decreaseDatabaseSize implementation in different drivers. Notice that in upcoming PRs we will change the implementation in postgres_driver.nim per partition truncation.

@Ivansete-status Ivansete-status self-assigned this Feb 20, 2024
Copy link

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.

Copy link

github-actions bot commented Feb 20, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2463

Built from 8f1390a

The new proc, decreaseDatabaseSize, will have different implementations
per each driver. For example, in future commits we will implement a size
retention policy thanks to partitions management, in Postgres.
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM!

Very useful policy.

Do we know how long does it take? Rough estimate?

@Ivansete-status
Copy link
Collaborator Author

LGTM!

Very useful policy.

Do we know how long does it take? Rough estimate?

Thanks for the review!
This week I'll submit a PR to tackle the "partition" approach in Postgres. The implementation is done but first of all, I will submit another previous PR with the database migration logic (pending implementation.)

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Yes, its a great step ahead!

One tiny naming issue for me, if you might consider. When reading first the code change from init to new was ambiguous until I did not check the code wider that the derived Time/Size/Capacity RetentionPolicies are defined as ref of RetentionPolicy.
Maybe it would help reading the code to use ...Ref naming of derived classes?
https://status-im.github.io/nim-style-guide/formatting.naming.html?highlight=new#naming-conventions-formattingnaming

@SionoiS
Copy link
Contributor

SionoiS commented Feb 21, 2024

@Ivansete-status I meant how long to reduce the DB by 20%. For ~1 million messages.

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.

Looks great! Thanks so much! 🤩

@Ivansete-status Ivansete-status merged commit d530528 into master Feb 22, 2024
10 checks passed
@Ivansete-status Ivansete-status deleted the refactor-size-retention-policy branch February 22, 2024 15:55
@Ivansete-status
Copy link
Collaborator Author

@Ivansete-status I meant how long to reduce the DB by 20%. For ~1 million messages.

ah ok! I haven't measured the time but it is much faster than deleting rows. By removing partitions we directly truncate many rows at once.

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

Successfully merging this pull request may close these issues.

None yet

4 participants