chore: simplify migration scripts and remove store legacy code #2894

Ivansete-status · 2024-07-10T23:32:39Z

Description

Simplify the migration scripts thanks to already having the timestamp data within the storedAt column.
Remove all the store legacy code as we are switching to store-v3 only

github-actions · 2024-07-10T23:37:11Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2894

Built from 27441e6

SionoiS

1000% Simpler but is it no longer a requirement to be backward compatible?

As for the migration, old messages timestamp would be all different after the migration would that not affect Status?

Ivansete-status · 2024-07-11T11:48:05Z

1000% Simpler but is it no longer a requirement to be backward compatible?

As for the migration, old messages timestamp would be all different after the migration would that not affect Status?

I think we are aiming for not backward compatible changes and the same happens in the Status changes. @richard-ramos - correct me if I'm wrong

gabrielmer

Ooh super happy to finally remove legacy store! We should also make sure tu remove it from the API documentation in https://github.com/waku-org/waku-rest-api

Regarding the migration script, there's something I don't quite understand.
Why are we dropping timestamp and renaming storedAt to timestamp instead of just dropping storedAt? are the current values in timestamp wrong?

Ivansete-status · 2024-07-11T12:38:05Z

Ooh super happy to finally remove legacy store! We should also make sure tu remove it from the API documentation in https://github.com/waku-org/waku-rest-api

Thanks for the reminder!

Regarding the migration script, there's something I don't quite understand. Why are we dropping timestamp and renaming storedAt to timestamp instead of just dropping storedAt? are the current values in timestamp wrong?

Both timestamp and storedAt fields keep exactly the same values. We can't remove the storedAt attribute first because it is part of the PK and hence we apply that movement.

gabrielmer

Amazing! Thanks so much! 😍

richard-ramos · 2024-07-11T12:57:56Z

Even tho status-go will then use v3 once PR status-im/status-go#5123 is merged, existing users that have not updated their status-app will continue to use storeV2 (since that's what their app's status-go version will use), so I'm not sure we should remove the legacy code yet, but keep it instead for a couple of months.

This reverts commit 7f3b471.

This reverts commit a63db95.

Ivansete-status · 2024-07-11T13:18:56Z

Thanks for the comment @richard-ramos !
Therefore, we need to keep the support to store-v2 until Stauts users have switched to store-v3 ( we consider at least one 6 weeks ( one Status release period ) of having the option to use store-v3 .)

NagyZoltanPeter

LGTM, yeah would have been to easy step to remove store v2 for now. But will come!

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

We want the nwaku node to simultaneously support store-v2 requests and store-v3 requests. Only legacy archive is in charge to archiving messages, and the archived information is suitable to fulfill both store-v2 and store-v3 needs.

github-actions · 2024-07-11T22:12:21Z

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.

Ivansete-status · 2024-07-11T22:49:47Z

1000% Simpler but is it no longer a requirement to be backward compatible?

Thanks again for the comments @SionoiS !
May take another look plz guys?

In the current changes we are simultaneously supporting store-v2 and store-v3

( cc @NagyZoltanPeter @gabrielmer )

gabrielmer

Amazingg PR! Really ingenious workaround to support simultaneously both store versions and keep the code nice and clean :)

Added just a small question and a super nitpick comment

waku/factory/node_factory.nim

gabrielmer · 2024-07-12T11:00:44Z

waku/factory/node_factory.nim

    let rateLimitSetting: RateLimitSetting =
      (conf.requestRateLimit, chronos.seconds(conf.requestRatePeriod))
-
    try:
      await mountLegacyStore(node, rateLimitSetting)


Here we are calling mountLegacyStore(node, rateLimitSetting) even in the cases of conf.legacyStore being false. Is this expected?

Thanks for that! It is corrected in 5b9871b

Co-authored-by: gabrielmer <101006718+gabrielmer@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 added 2 commits July 11, 2024 01:26

removing store legacy code

a63db95

simplify postgres migration scripts

65423d4

Ivansete-status self-assigned this Jul 10, 2024

Ivansete-status added 2 commits July 11, 2024 02:15

chat2 adjust compilation issues

f1e4c87

all_tests_waku minor compilation fix

7f3b471

Ivansete-status marked this pull request as ready for review July 11, 2024 10:59

Ivansete-status requested review from SionoiS, NagyZoltanPeter, darshankabariya and gabrielmer July 11, 2024 10:59

SionoiS approved these changes Jul 11, 2024

View reviewed changes

gabrielmer reviewed Jul 11, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

gabrielmer self-requested a review July 11, 2024 12:44

gabrielmer approved these changes Jul 11, 2024

View reviewed changes

Ivansete-status mentioned this pull request Jul 11, 2024

chore: delete references to store-v2 waku-org/waku-rest-api#14

Open

Ivansete-status added 2 commits July 11, 2024 15:14

Revert "all_tests_waku minor compilation fix"

a41336f

This reverts commit 7f3b471.

Revert "removing store legacy code"

f5af6cf

This reverts commit a63db95.

NagyZoltanPeter approved these changes Jul 11, 2024

View reviewed changes

Ivansete-status added 6 commits July 11, 2024 16:52

posgres legacy: stop using the storedAt field

33524b9

migration script 6: we still need the id column

7260c71

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

649cf8d

waku_node: try to use wakuLegacyArchive if wakuArchive is nil

e2a08cd

postgres_driver: adding temporary code until store-v2 is removed

4d02db5

gabrielmer reviewed Jul 12, 2024

View reviewed changes

Ivansete-status and others added 2 commits July 12, 2024 13:28

move comment waku/factory/node_factory.nim

51b63e9

Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com>

node_factory: mountLegacyStore only if conf.legacyStore is true

5b9871b

SionoiS approved these changes Jul 12, 2024

View reviewed changes

gabrielmer self-requested a review July 12, 2024 11:50

gabrielmer approved these changes Jul 12, 2024

View reviewed changes

Ivansete-status merged commit 16b692c into chore--archive-drivers-refactor Jul 12, 2024
8 of 10 checks passed

Ivansete-status deleted the chore--archive-drivers-refactor-2 branch July 12, 2024 12:57

Ivansete-status restored the chore--archive-drivers-refactor-2 branch July 12, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: simplify migration scripts and remove store legacy code #2894

chore: simplify migration scripts and remove store legacy code #2894

Ivansete-status commented Jul 10, 2024

github-actions bot commented Jul 10, 2024 •

edited

Loading

SionoiS left a comment

Ivansete-status commented Jul 11, 2024

gabrielmer left a comment

This comment was marked as duplicate.

Ivansete-status commented Jul 11, 2024

gabrielmer left a comment

richard-ramos commented Jul 11, 2024

Ivansete-status commented Jul 11, 2024

NagyZoltanPeter left a comment

github-actions bot commented Jul 11, 2024

Ivansete-status commented Jul 11, 2024

gabrielmer left a comment

gabrielmer Jul 12, 2024

Ivansete-status Jul 12, 2024

chore: simplify migration scripts and remove store legacy code #2894

chore: simplify migration scripts and remove store legacy code #2894

Conversation

Ivansete-status commented Jul 10, 2024

Description

github-actions bot commented Jul 10, 2024 • edited Loading

SionoiS left a comment

Choose a reason for hiding this comment

Ivansete-status commented Jul 11, 2024

gabrielmer left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Ivansete-status commented Jul 11, 2024

gabrielmer left a comment

Choose a reason for hiding this comment

richard-ramos commented Jul 11, 2024

Ivansete-status commented Jul 11, 2024

NagyZoltanPeter left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 11, 2024

Ivansete-status commented Jul 11, 2024

gabrielmer left a comment

Choose a reason for hiding this comment

gabrielmer Jul 12, 2024

Choose a reason for hiding this comment

Ivansete-status Jul 12, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 10, 2024 •

edited

Loading