-
Notifications
You must be signed in to change notification settings - Fork 333
ConversationStore: Implement migration to postgres #4810
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
Conversation
c32b073 to
4e1c43c
Compare
905ece0 to
ea4a8bd
Compare
4e1c43c to
619999d
Compare
ea4a8bd to
ce9435e
Compare
2b79ef3 to
5090c28
Compare
4ca15c9 to
e44c50d
Compare
… during migration Exceptions: 1. Getting a paginated list of qualified conv Ids 2. It will not work well if the migration fails to delete conv data from Cassandra after copying it to postgres These problems will be solved in following commits
When fetching `maxIds + 1` convs, it can happen that a user has exactly those many convs left, in this case the `hasMore` field of the page would be false, but we'd be sending a truncated list of convs.
This is done by making the pagingState encode last conversation Id served. The store effect has a new action to list only remote conv ids. The `GetConverastionIds` action has been removed and implemented generally using `GetLocalConverastionIds` and `GetRemoteConversationIds`. This makes `MultiTabelPage` type obsolete for conv ids, but its still kept around so we don't break any APIs.
…nv in postgres This is consistent with creating new conversations in postgres. This way when the migration is complete already running galley instances won't create more data in Cassandra
…tatuses of a user
…ndra are never read Exception: Listing local conversation ids and listing team conversation ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for migrating conversation data from Cassandra to PostgreSQL, enabling channel search and management in team UI. The migration occurs in three phases: preparation (write to both DBs), active migration (background worker moves existing data), and PostgreSQL-only operation.
Key changes:
- Adds a new
MigrationToPostgresqlstorage location option for phased migration - Implements background worker to migrate conversations and remote member data
- Introduces migration locks to prevent race conditions during concurrent operations
Reviewed Changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| services/galley/src/Galley/Options.hs | Adds new MigrationToPostgresql storage option with documentation |
| services/galley/src/Galley/App.hs | Adds polysemy-conc dependency and MigrationError handling |
| libs/wire-subsystems/src/Wire/ConversationStore/*.hs | Implements dual-write interpreter and migration logic |
| services/background-worker/src/Wire/*.hs | Adds migration worker with Prometheus metrics |
| charts/background-worker/*.yaml | Adds Cassandra Galley connection and PostgreSQL config |
| integration/test/Test/Conversation/Migration.hs | Comprehensive migration tests for MLS and Proteus conversations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/wire-subsystems/src/Wire/ConversationStore/Migration/Cleanup.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pcapriotti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
| CREATE TABLE conversation_migration_pending_deletes ( | ||
| typ text NOT NULL, | ||
| id uuid NOT NULL, | ||
| PRIMARY KEY (typ, id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no foreign key constraint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to track pending deletes in Cassandra, in case the conv gets deleted from Postgres while its pending in cassandra we don't want to also forget to delete it from Cassandra. So I think it necessary to not create a foreign key, because that will either block conv from being deleted in postgres or if we do a cascading delete, make us forget that this conv needs to be deleted in Cassandra and the migration code will revive this conv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Because sometimes we don't have to get an empty page to realize we're at the end
13322d3 to
ca61c3c
Compare
Also stop any throttling, OOMKilling
ca61c3c to
192ce20
Compare
e775514 to
e08cf96
Compare
378682d to
0f3c1a2
Compare
… in same order as IDs
https://wearezeta.atlassian.net/browse/WPB-20751
Checklist
changelog.d