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.

Sign up for GitHub

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

Jump to bottom

Feat split migrations #8144

Open

PineappleIOnic wants to merge 9 commits into refactor-usage-sn from feat-split-migrations

+352 −102

Member

PineappleIOnic commented May 17, 2024 •

edited

What does this PR do?

This PR splits migrations out so it can be handled by multiple workers

Test Plan

Tested locally for Appwrite, still need to test firebase, and other adapters. Will tick off as I go

Firebase
Supabase
NHost

This is a DB Breaking change, a migration will be needed.

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

PineappleIOnic added 5 commits

May 17, 2024 15:27


          Refactor migrations to allow for more concurrency

eff09b6


          Clean up code

4d76abe


          Merge branch 'main' into feat-split-migrations

fdaf79e


          Run Linters

abd4772


          Merge branch 'main' of https://github.com/appwrite/appwrite into feat…

d20d322

…-split-migrations

christyjacob4 reviewed

View reviewed changes

src/Appwrite/Utopia/Response/Model/Migration.php

@@ @@ -34,12 +34,6 @@ public function __construct() @@
                               'default' => '',
                               'example' => 'pending',
                           ])
-                          ->addRule('stage', [

Member

christyjacob4 May 21, 2024

whats the difference between stage and status? why is this removed?

This needs to be released as a patch version so we should not have breaking changes. If it has to be removed, then we need a response filter for it

Member Author

PineappleIOnic May 28, 2024

I've added a response filter, please re-review

christyjacob4 reviewed

View reviewed changes

docker-compose.yml

Comment on lines -627 to -651

		- _APP_MIGRATIONS_FIREBASE_CLIENT_ID
		- _APP_MIGRATIONS_FIREBASE_CLIENT_SECRET

Member

christyjacob4 May 21, 2024

why was this removed?

Member Author

PineappleIOnic May 24, 2024

Since we're never rolling out firebase OAuth so I just removed it while I was there


          Add response filter and remove event test

aa856b6

PineappleIOnic requested a review from christyjacob4

May 28, 2024 03:40

PineappleIOnic changed the base branch from main to refactor-usage-sn

May 28, 2024 05:37


          Merge branch 'refactor-usage-sn' into feat-split-migrations

6f0a9a8

Member Author

PineappleIOnic commented May 28, 2024

@christyjacob4 I rebased this to refactor-usage-sn so we can get this onto cloud easier


          Run Linter

47ecb33

christyjacob4 reviewed

View reviewed changes

app/config/collections.php Outdated Show resolved Hide resolved


          Update collections.php

2bf291f

PineappleIOnic requested a review from christyjacob4

May 30, 2024 02:29

christyjacob4 reviewed

View reviewed changes

app/config/collections.php

@@ @@ -4138,6 +4117,106 @@ @@
                           ]
                       ],
                   ],
+                  'groupMigrations' => [

Member

christyjacob4 May 31, 2024

Suggested change

      
                'groupMigrations' => [
          
                'migrationsGroup' => [

christyjacob4 reviewed

View reviewed changes

app/controllers/api/migrations.php

Comment on lines +44 to +47

+                      if (!empty(array_intersect(
+                          $groupResources,
+                          $resources
+                      ))) {

Member

christyjacob4 May 31, 2024

Lets move this to a variable and reuse it on L53

christyjacob4 reviewed

View reviewed changes

app/controllers/api/migrations.php

                       $migration = $dbForProject->createDocument('migrations', new Document([
                           '$id' => ID::unique(),
                           'status' => 'pending',
-                          'stage' => 'init',

Member

christyjacob4 May 31, 2024

lets keep the stage attribute since its required. We can attempt to remove it during the 1.6 migrations for example

christyjacob4 reviewed

View reviewed changes

src/Appwrite/Platform/Workers/Migrations.php

                           roles: $target['roles'],
                       );
-                      return $this->dbForProject->updateDocument('migrations', $migration->getId(), $migration);
+                      return $this->dbForProject->updateDocument($migration->getCollection(), $migration->getId(), $migration);

Member

christyjacob4 May 31, 2024

is this change required?

christyjacob4 reviewed

View reviewed changes

src/Appwrite/Platform/Workers/Migrations.php

Comment on lines 257 to +261

                           $log->addBreadcrumb(new Breadcrumb("debug", "migration", "Migration hit stage 'processing'", \microtime(true)));
-                          $this->updateMigrationDocument($migrationDocument, $projectDocument);
-                          $log->addTag('type', $migrationDocument->getAttribute('source'));
+                          $this->updateMigrationDocument($migration, $projectDocument);
+                          $log->addTag('type', $migration->getAttribute('source'));

Member

christyjacob4 May 31, 2024

Lets group the logger related statements together and the migrations related statements together

christyjacob4 reviewed

View reviewed changes

src/Appwrite/Platform/Workers/Migrations.php

Comment on lines +281 to +296

+                          switch ($group->getAttribute('group')) {
+                              case Transfer::GROUP_AUTH:
+                                  $resources = array_intersect(Transfer::GROUP_AUTH_RESOURCES, $resources);
+                                  break;
+                              case Transfer::GROUP_STORAGE:
+                                  $resources = array_intersect(Transfer::GROUP_STORAGE_RESOURCES, $resources);
+                                  break;
+                              case Transfer::GROUP_DATABASES:
+                                  $resources = array_intersect(Transfer::GROUP_DATABASES_RESOURCES, $resources);
+                                  break;
+                              case Transfer::GROUP_FUNCTIONS:
+                                  $resources = array_intersect(Transfer::GROUP_FUNCTIONS_RESOURCES, $resources);
+                                  break;
+                              default:
+                                  throw new Exception('Migration worker was initialized with unknown group');
+                          }

Member

christyjacob4 May 31, 2024

You already did an array_intersect in the API level. This doesn't seem to be required

christyjacob4 reviewed

View reviewed changes

src/Appwrite/Platform/Workers/Migrations.php


		$status = $document->getAttribute('status', 'processing');

		if ($status == 'processing' \|\| $status == 'pending') {

Member

christyjacob4 May 31, 2024

Suggested change

      
                            if ($status == 'processing' || $status == 'pending') {
          
                            if ($status === 'processing' || $status === 'pending') {

christyjacob4 reviewed

View reviewed changes

src/Appwrite/Platform/Workers/Migrations.php

Comment on lines +355 to +357

+                              if ($status == 'failed') {
+                                  break;
+                              }

Member

christyjacob4 May 31, 2024

Suggested change

      
                            if ($status == 'failed') {
          
                                break;
          
                            }
          
                            if ($status === 'failed') {
          
                               $result = 'failed';
          
                                break;
          
                            }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment