-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support redis migrations #8898
base: develop
Are you sure you want to change the base?
Support redis migrations #8898
Changes from 1 commit
7a32e8b
78338c7
10e3a1d
fae0974
88ef5a0
dc1f3bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
# Copyright (C) 2025 CVAT.ai Corporation | ||
# | ||
# SPDX-License-Identifier: MIT | ||
|
||
import importlib.util as importlib_util | ||
import sys | ||
from pathlib import Path | ||
from typing import cast | ||
|
||
from django.conf import settings | ||
from django.core.management.base import BaseCommand | ||
|
||
from cvat.apps.engine.models import RedisMigration | ||
from cvat.apps.engine.redis_migrations import BaseMigration | ||
|
||
|
||
def get_migration_class(module_name: str, file_path: Path) -> BaseMigration: | ||
spec = importlib_util.spec_from_file_location(module_name, file_path) | ||
module = importlib_util.module_from_spec(spec) | ||
spec.loader.exec_module(module) | ||
MigrationClass = getattr(module, "Migration", None) | ||
if not MigrationClass or not issubclass(MigrationClass, BaseMigration): | ||
raise Exception(f"Invalid migration: {file_path}") | ||
|
||
return MigrationClass | ||
|
||
|
||
class Command(BaseCommand): | ||
help = "Applies Redis migrations and records them in the database" | ||
|
||
def handle(self, *args, **options) -> None: | ||
migrations_dir = Path(settings.REDIS_MIGRATIONS_ROOT) | ||
applied_migrations = RedisMigration.objects.all().values_list("name") | ||
|
||
for migration_file in sorted(migrations_dir.glob("[0-9]*.py")): | ||
migration_name = migration_file.stem | ||
if migration_name in applied_migrations: | ||
continue | ||
|
||
migration = get_migration_class(module_name=migration_name, file_path=migration_file) | ||
try: | ||
migration.run() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewing purposes, it would be useful to see an example of a migration class. |
||
RedisMigration.objects.create(name=migration_name) | ||
self.stdout.write( | ||
self.style.SUCCESS(f"Successfully applied migration: {migration_name}") | ||
) | ||
except Exception as e: | ||
self.stderr.write(self.style.ERROR(f"Failed to apply migration: {migration_name}")) | ||
self.stderr.write(str(e)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A full traceback would be useful here, and not just the message. |
||
break | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't an exception cause the command to exit with a failure status? Right now it seems to be exiting successfully. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# Generated by Django 4.2.16 on 2025-01-03 17:08 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("engine", "0086_profile_has_analytics_access"), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name="RedisMigration", | ||
fields=[ | ||
( | ||
"id", | ||
models.AutoField( | ||
auto_created=True, primary_key=True, serialize=False, verbose_name="ID" | ||
), | ||
), | ||
("name", models.CharField(max_length=256)), | ||
("applied_date", models.DateTimeField(auto_now_add=True)), | ||
], | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1345,3 +1345,9 @@ class RequestSubresource(TextChoices): | |
ANNOTATIONS = "annotations" | ||
DATASET = "dataset" | ||
BACKUP = "backup" | ||
|
||
|
||
class RedisMigration(models.Model): | ||
# todo: redis_inmem/redis_ondisk | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by this? |
||
name = models.CharField(max_length=256) | ||
applied_date = models.DateTimeField(auto_now_add=True) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
from abc import ABCMeta, abstractmethod | ||
|
||
|
||
class BaseMigration(metaclass=ABCMeta): | ||
|
||
@staticmethod | ||
@abstractmethod | ||
def run() -> None: ... |
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 seems a bit inflexible, because it only allows for one migration root. What will we do if we need to add migrations to the Enterprise repository, for example?
The database migration functionality in Django loads migrations from each app; perhaps we could do the same here?