Skip to content
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

Feature: Add support for running migration in batches #7755

Open
wants to merge 7 commits into
base: epic/campaigns
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/Campaigns/Migrations/Donations/AddCampaignId.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
use Give\Donations\ValueObjects\DonationMetaKeys;
use Give\Framework\Database\DB;
use Give\Framework\Database\Exceptions\DatabaseQueryException;
use Give\Framework\Migrations\Contracts\BatchMigration;
use Give\Framework\Migrations\Contracts\Migration;
use Give\Framework\Migrations\Exceptions\DatabaseMigrationException;

/**
* @unreleased
*/
class AddCampaignId extends Migration
class AddCampaignId extends Migration implements BatchMigration
{
/**
* @inheritDoc
Expand Down Expand Up @@ -41,7 +42,7 @@ public static function timestamp(): string
* @inheritDoc
* @throws DatabaseMigrationException
*/
public function run()
public function runBatch($batchNumber)
{
$relationships = [];

Expand All @@ -63,6 +64,8 @@ public function run()
[DonationMetaKeys::FORM_ID(), 'formId']
)
->where('post_type', 'give_payment')
->offset($batchNumber)
->limit($this->getBatchSize())
->getAll();

$donationMeta = [];
Expand All @@ -86,7 +89,26 @@ public function run()
->insert($donationMeta, ['%d', '%s', '%d']);
}
} catch (DatabaseQueryException $exception) {
throw new DatabaseMigrationException("An error occurred while adding campaign ID to the donation meta table", 0, $exception);
throw new DatabaseMigrationException("An error occurred while adding campaign ID to the donation meta table",
0, $exception);
}
}

/**
* @inheritDoc
*/
public function getItemsCount(): int
{
return DB::table('posts')
->where('post_type', 'give_payment')
->count();
}

/**
* @inheritDoc
*/
public function getBatchSize(): int
{
return 50;
}
}
30 changes: 30 additions & 0 deletions src/Framework/Migrations/Contracts/BatchMigration.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php

namespace Give\Framework\Migrations\Contracts;

/**
* @unreleased
*/
interface BatchMigration
{
/**
* @unreleased
*
* Get the number of items per batch
*/
public function getBatchSize(): int;

/**
* @unreleased
*
* Get the total items count
*/
public function getItemsCount(): int;

/**
* @unreleased
*
* Run batch
*/
public function runBatch($batchNumber);
}
8 changes: 7 additions & 1 deletion src/Framework/Migrations/Contracts/Migration.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,15 @@ abstract class Migration
/**
* Bootstrap migration logic.
*
* @unreleased this method is now optional if migration class implements BatchMigration interface
* @since 2.9.0
*/
abstract public function run();
public function run()
{
if ( ! is_subclass_of($this, BatchMigration::class)) {
throw new RuntimeException('run method is not defined.');
}
}
Comment on lines -21 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid this if we can. This sort of thing can't be caught by static analysis, so the only way to know if there's an issue is at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch everything to inheritance we shouldn't need to do this.


/**
* Return a unique identifier for the migration
Expand Down
94 changes: 81 additions & 13 deletions src/Framework/Migrations/MigrationsRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
namespace Give\Framework\Migrations;

use Exception;
use Give\Framework\Database\DB;
use Give\Framework\Database\Exceptions\DatabaseQueryException;
use Give\Framework\Migrations\Contracts\BatchMigration;
use Give\Framework\Migrations\Contracts\Migration;
use Give\Framework\Support\Facades\ActionScheduler\AsBackgroundJobs;
use Give\Log\Log;
use Give\MigrationLog\MigrationLogFactory;
use Give\MigrationLog\MigrationLogRepository;
Expand Down Expand Up @@ -67,12 +70,11 @@ public function __construct(
/**
* Run database migrations.
*
* @since 2.9.0
* @unreleased add support for batch processing
* @since 2.9.0
*/
public function run()
{
global $wpdb;

if ( ! $this->hasMigrationToRun()) {
return;
}
Expand All @@ -94,7 +96,7 @@ public function run()

ksort($migrations);

foreach ($migrations as $key => $migrationClass) {
foreach ($migrations as $migrationClass) {
$migrationId = $migrationClass::id();

if (in_array($migrationId, $this->completedMigrations, true)) {
Expand All @@ -103,19 +105,30 @@ public function run()

$migrationLog = $this->migrationLogFactory->make($migrationId);

// Begin transaction
$wpdb->query('START TRANSACTION');

try {
/** @var Migration $migration */
$migration = give($migrationClass);

$migration->run();

// Save migration status
$migrationLog->setStatus(MigrationLogStatus::SUCCESS);
if (is_subclass_of($migration, BatchMigration::class)) {
$status = $this->runBatch($migration);

if ($status === MigrationLogStatus::RUNNING) {
give()->notices->register_notice(
[
'id' => $migrationId,
'description' => esc_html__('Running DB migration: ' . $migration::title(), 'give'),
]
);
break;
}

$migrationLog->setStatus($status);
} else {
$migration->run();
$migrationLog->setStatus(MigrationLogStatus::SUCCESS);
}
} catch (Exception $exception) {
$wpdb->query('ROLLBACK');
DB::rollback();

$migrationLog->setStatus(MigrationLogStatus::FAILED);
$migrationLog->setError($exception);
Expand Down Expand Up @@ -152,7 +165,7 @@ public function run()
}

// Commit transaction if successful
$wpdb->query('COMMIT');
DB::commit();
}
}

Expand All @@ -167,4 +180,59 @@ public function hasMigrationToRun()
{
return (bool)array_diff($this->migrationRegister->getRegisteredIds(), $this->completedMigrations);
}

/**
* Run migration batch
*
* @unreleased
* @throws Exception
*/
public function runBatch(BatchMigration $migration): string
{
$group = $migration::id();
$actionHook = 'givewp-batch-' . $group;

add_action($actionHook, function ($batchNumber) use ($migration) {
DB::beginTransaction();

try {
$migration->runBatch($batchNumber);

DB::commit();
} catch (Exception $e) {
DB::rollback();
throw new Exception($e->getMessage(), 0, $e);
}
});

$actions = AsBackgroundJobs::getActionsByGroup($group);

// register actions - initial run
if (empty($actions)) {
$batches = ceil($migration->getItemsCount() / $migration->getBatchSize());

for ($i = 0; $i < $batches; $i++) {
AsBackgroundJobs::enqueueAsyncAction($actionHook, [$i], $group);
}

return MigrationLogStatus::RUNNING;
}

$pendingActions = AsBackgroundJobs::getActionsByGroup($group, 'pending');

if ( ! empty($pendingActions)) {
return MigrationLogStatus::RUNNING;
}

$failedActions = AsBackgroundJobs::getActionsByGroup($group, 'failed');

if ( ! empty($failedActions)) {
return MigrationLogStatus::FAILED;
}

// todo: discuss deleting actions
// AsBackgroundJobs::deleteActionsByGroup($group);

return MigrationLogStatus::SUCCESS;
}
}
4 changes: 3 additions & 1 deletion src/Framework/Migrations/MigrationsServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public function boot()
{
Hooks::addAction('admin_init', ManualMigration::class, '__invoke', 0);
Hooks::addAction('admin_init', MigrationsRunner::class, 'run', 0);
Hooks::addAction('give_upgrades', MigrationsRunner::class, 'run', 0);
//Hooks::addAction('give_upgrades', MigrationsRunner::class, 'run', 0);
// running batch actions via cron doesn't trigger give_upgrades and all registered actions fail
Comment on lines -33 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this break the existing migrations that are using the old upgrade system?

Hooks::addAction('action_scheduler_init', MigrationsRunner::class, 'run');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@
/**
* @since 3.6.0
*
* @unreleased
* - added enqueueAction method
* - switch parameter $status position with $returnFormat position
*
* @method static int enqueueAsyncAction(string $hook, array $args, string $group, bool $unique = false, int $priority = 10)
* @method static int enqueueAction(int $timestamp, string $hook, array $args, string $group, bool $unique = false, int $priority = 10)
* @method static array getActionByHookArgsGroup(string $hook, array $args, string $group, string $returnFormat = OBJECT, string $status = '')
* @method static array getActionsByGroup(string $group, string $returnFormat = OBJECT, string $status = '')
* @method static array getActionsByGroup(string $group, string $status = '', string $returnFormat = OBJECT)
* @method static int deleteActionsByGroup(string $group, string $status = '')
*/
class AsBackgroundJobs extends Facade
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,25 @@ public function enqueueAsyncAction(
}
}

/**
* @unreleased
*/
public function enqueueAction(
int $timestamp,
string $hook,
array $args,
string $group,
bool $unique = false,
int $priority = 10
): int {
$enqueuedAction = $this->getActionByHookArgsGroup($hook, $args, $group, 'ids');
if (empty($enqueuedAction)) {
return as_schedule_single_action($timestamp, $hook, $args, $group, $unique, $priority);
}

return $enqueuedAction[0];
}
Comment on lines +45 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a description for this. It looks like it conditionally enqueues a single action so long as that action isn't already queued? We may want to call this safelyEnqueueAction or something like that.


/**
* @since 3.6.0
*
Expand Down Expand Up @@ -76,14 +95,15 @@ public function getActionByHookArgsGroup(

/**
* @since 3.6.0
* @unreleased - switch parameter $status position with $returnFormat position
*
* @param string $group The group to assign this job to.
* @param string $returnFormat OBJECT, ARRAY_A, or ids.
* @param string $status ActionScheduler_Store::STATUS_COMPLETE or ActionScheduler_Store::STATUS_PENDING
*
* @return array
*/
public function getActionsByGroup(string $group, string $returnFormat = OBJECT, string $status = ''): array
public function getActionsByGroup(string $group, string $status = '', string $returnFormat = OBJECT): array
{
$args = [
'group' => $group,
Expand Down
3 changes: 3 additions & 0 deletions src/MigrationLog/MigrationLogStatus.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@
* Class MigrationLogStatus
* @package Give\MigrationLog
*
* @unreleased add running status
* @since 2.10.0
*/
class MigrationLogStatus
{
const SUCCESS = 'success';
const FAILED = 'failed';
const PENDING = 'pending';
const RUNNING = 'running';

/**
* Get default migration status
Expand All @@ -35,6 +37,7 @@ public static function getAll()
MigrationLogStatus::SUCCESS => esc_html__('Success', 'give'),
MigrationLogStatus::FAILED => esc_html__('Failed', 'give'),
MigrationLogStatus::PENDING => esc_html__('Pending', 'give'),
MigrationLogStatus::RUNNING => esc_html__('Running', 'give'),
];
}

Expand Down
Loading