-
Notifications
You must be signed in to change notification settings - Fork 0
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
DB Versioning & Migration Tools (GSI-1462) #159
base: main
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 13817926961Details
💛 - Coveralls |
…for auto_finalize()
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. Made some suggestions.
Co-authored-by: Christoph Zwerschke <[email protected]>
error = MigrationStepError( | ||
current_ver=version - 1, | ||
target_ver=self.target_ver, | ||
) |
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.
Does current_ver
have the correct value for both backward and forward here? Shouldn't it simply be version
for unapplying?
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.
You might be right. I'll add a small test for it
index.pop("v", "") | ||
index.pop("ns", "") |
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.
What does this do?
Co-authored-by: Thomas Zajac <[email protected]>
This is the official implementation of the DB migration prototype implemented for the IFRS/DCS in the file services monorepo.
Most of the functionality is the same, but some small improvements/tweaks have been added.
PR Info
Changes from prototype
Batch Processing:
migrate_docs_in_collection()
now takes abatch_size
parameter to control the batch size used by the cursor. When inserting the migrated documents, we now call.insert_many()
instead of making sequential calls to.insert()
, and the size of the inserts is also controlled bybatch_size
. Thebatch_size
parameter defaults to 1000. Note that MongoDB will only return batches of up to 16 MB, so the batch size only limits the number of documents retrieved within that limit (the default for MongoDB is 101).Index Copying:
copy_indexes()
is now functional, and will copy the indexes between two collections directly. There's an automatic/auto-tracking version calledauto_copy_indexes()
that accepts the normal collection names and handles prefixing and remembers which collections have had the indexes copied (similar to the staging process). Under the hood,auto_copy_indexes()
callscopy_indexes()
.Configurable Timeout for Waiting Instances:
In the case where multiple service instances exist and all but one must wait for the migration process to finish, the prototype provided no way for those services to timeout. This PR adds a config option,
migration_max_wait_sec
. If set, waiting services will raise aMigrationTimeoutError
. The value is optional, though, and setting it toNone
will allow for infinite waiting.Timestamp Format:
Timestamps for the DB Version Records and the lock document are now UTC Datetimes, while the prototype used strings.
There's a small hiccup with this since the IFRS and DCS already inserted a few documents with the string format, but we will prevail.
Tests for the Migration Tools
Currently, the following have been added:
unapply
is not defined on a migration definition while migrating downbatch_size
to ensure all docs are migratedauto_finalize()
context manager in successful and error cases.Items implemented from prototype wishlist:
✅ Batch processing
✅ Index copying (need to understand this better)
✅ Better documentation
✅ Limits for retries?
✅ Testing to cover most the edge cases along with unit testing to better catch problems with helper functions
Delayed or Scratched:
⏲️ Better logging/error handling (did some small changes, but mostly left it as is)
⏲️ Better metrics (e.g. # documents affected)
Reversible subclass thing alternate implementationautomatically pass config to instances ofMigrationDefinition
- only an inconvenience for testing thoughUsage (Bonus Info)
Step 1: The logic for every migration should subclass
MigrationDefinition
and, usually,Reversible
as well:The naming convention is V<version>Migration. Migrations occur at the database level, meaning there should only be one migration class for every version of the database -- each migration class should migrate all pertinent collections within the database:
V4Migration
V4UsersMigration
,MySuperRadMigrationClass
Step 2: Define the current database version and the migration map.
Step 3: Instantiate the
MigrationManager
viaasync with
, then call.migrate_or_wait()
:Step 4: Perform the DB migrations/version check sometime before firing up the actual service code:
Notes
The
MigrationDefinition
class defines some useful logic for common operations, such as "staging" migrated data (where we rename the collections to swap the new data with the old), dropping new collections, generating temp table prefixing, applying an update function to each doc in a collection with validation, and a context manager to automate staging/dropping/error case cleanup.The
MigrationManager
handles higher-level responsibilities and is ignorant of migration implementation details. It cares about whether or not the database is set up for migrations, which migrations it needs to run to get from version X to version Y, and running said migrations. It handles the database "lock" document to ensure only one service instance ever performs migrations. It also maintains a record of completed migrations, which is stored in the configureddb_version_collection
.Solving the catch-22 of making sure only one instance initializes the lock/versioning collection is straightforward: the code inserts a lock document with a fixed ID. If the result is a
DuplicateKeyError
error, stop and enter the waiting cycle (another instance was faster), otherwise continue with the init & migration logic. The service instance that performs the initialization will already have the lock acquired after finishing setup and can roll right into executing any migrations.