From 87d3f103967bcfb30d69cedd39a81e7e9a39ff05 Mon Sep 17 00:00:00 2001 From: Aaron Date: Thu, 17 May 2012 16:34:52 -0700 Subject: [PATCH] [FileBackend] MultiWrite code improvements and sanity checks. * Automatically override a few sub-backend settings in FileBackendMultiWrite. We should ignore locking, journaling, or read-only settings of sub-backends. Journaling is now done just for master sub-backend paths (under the proxy backend name). We don't need to log for each sub-backend, which is a waste of disk space. * Changed FileBackendMultiWrite::doOperationsInternal() to be similiar to the other write functions. It now uses doOperations() of the sub-backends rather than manual code. This makes settings like 'concurrency' easier to manage; we might want to have some sub-backends with varying setings for that. * Made FileBackendMultiWrite::doQuickOperationsInternal() compliant with docs. * Added CHECK_SHA1 option for consistency checking. * Fixed function visibility in two places. * Improved multiwrite backend tests by calling consistencyCheck(). Change-Id: Iac7bfe10c77ecd069fb9ef0ec26a01512f5f4eea --- includes/filerepo/backend/FileBackend.php | 4 +- .../backend/FileBackendMultiWrite.php | 134 +++++++++--------- .../filerepo/backend/FileBackendStore.php | 2 +- .../includes/filerepo/FileBackendTest.php | 17 +++ 4 files changed, 91 insertions(+), 66 deletions(-) diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 788bcd73dd..de61ef6b22 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -103,7 +103,9 @@ abstract class FileBackend { ? $config['lockManager'] : LockManagerGroup::singleton()->get( $config['lockManager'] ); $this->fileJournal = isset( $config['fileJournal'] ) - ? FileJournal::factory( $config['fileJournal'], $this->name ) + ? ( ( $config['fileJournal'] instanceof FileJournal ) + ? $config['fileJournal'] + : FileJournal::factory( $config['fileJournal'], $this->name ) ) : FileJournal::factory( array( 'class' => 'NullFileJournal' ), $this->name ); $this->readOnly = isset( $config['readOnly'] ) ? (string)$config['readOnly'] diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php index 2fc9d8ed3b..c307759a65 100644 --- a/includes/filerepo/backend/FileBackendMultiWrite.php +++ b/includes/filerepo/backend/FileBackendMultiWrite.php @@ -48,9 +48,12 @@ class FileBackendMultiWrite extends FileBackend { /* Possible internal backend consistency checks */ const CHECK_SIZE = 1; const CHECK_TIME = 2; + const CHECK_SHA1 = 4; /** * Construct a proxy backend that consists of several internal backends. + * Locking, journaling, and read-only checks are handled by the proxy backend. + * * Additional $config params include: * 'backends' : Array of backend config and multi-backend settings. * Each value is the config used in the constructor of a @@ -61,9 +64,11 @@ class FileBackendMultiWrite extends FileBackend { * the config of that backend as a template. * Values specified here take precedence. * 'syncChecks' : Integer bitfield of internal backend sync checks to perform. - * Possible bits include self::CHECK_SIZE and self::CHECK_TIME. + * Possible bits include the FileBackendMultiWrite::CHECK_* constants. + * There are constants for SIZE, TIME, and SHA1. * The checks are done before allowing any file operations. * @param $config Array + * @throws MWException */ public function __construct( array $config ) { parent::__construct( $config ); @@ -81,17 +86,23 @@ class FileBackendMultiWrite extends FileBackend { throw new MWException( "Two or more backends defined with the name $name." ); } $namesUsed[$name] = 1; - if ( !isset( $config['class'] ) ) { - throw new MWException( 'No class given for a backend config.' ); - } - $class = $config['class']; - $this->backends[$index] = new $class( $config ); + // Alter certain sub-backend settings for sanity + unset( $config['readOnly'] ); // use proxy backend setting + unset( $config['fileJournal'] ); // use proxy backend journal + $config['lockManager'] = 'nullLockManager'; // lock under proxy backend if ( !empty( $config['isMultiMaster'] ) ) { if ( $this->masterIndex >= 0 ) { throw new MWException( 'More than one master backend defined.' ); } - $this->masterIndex = $index; + $this->masterIndex = $index; // this is the "master" + $config['fileJournal'] = $this->fileJournal; // log under proxy backend } + // Create sub-backend object + if ( !isset( $config['class'] ) ) { + throw new MWException( 'No class given for a backend config.' ); + } + $class = $config['class']; + $this->backends[$index] = new $class( $config ); } if ( $this->masterIndex < 0 ) { // need backends and must have a master throw new MWException( 'No master backend defined.' ); @@ -108,27 +119,14 @@ class FileBackendMultiWrite extends FileBackend { final protected function doOperationsInternal( array $ops, array $opts ) { $status = Status::newGood(); - $performOps = array(); // list of FileOp objects - $paths = array(); // storage paths read from or written to - // Build up a list of FileOps. The list will have all the ops - // for one backend, then all the ops for the next, and so on. - // These batches of ops are all part of a continuous array. - // Also build up a list of files read/changed... - foreach ( $this->backends as $index => $backend ) { - $backendOps = $this->substOpBatchPaths( $ops, $backend ); - // Add on the operation batch for this backend - $performOps = array_merge( $performOps, - $backend->getOperationsInternal( $backendOps ) ); - if ( $index == 0 ) { // first batch - // Get the files used for these operations. Each backend has a batch of - // the same operations, so we only need to get them from the first batch. - $paths = $backend->getPathsToLockForOpsInternal( $performOps ); - // Get the paths under the proxy backend's name - $paths['sh'] = $this->unsubstPaths( $paths['sh'] ); - $paths['ex'] = $this->unsubstPaths( $paths['ex'] ); - } - } + $mbe = $this->backends[$this->masterIndex]; // convenience + // Get the paths to lock from the master backend + $realOps = $this->substOpBatchPaths( $ops, $mbe ); + $paths = $mbe->getPathsToLockForOpsInternal( $mbe->getOperationsInternal( $realOps ) ); + // Get the paths under the proxy backend's name + $paths['sh'] = $this->unsubstPaths( $paths['sh'] ); + $paths['ex'] = $this->unsubstPaths( $paths['ex'] ); // Try to lock those files for the scope of this function... if ( empty( $opts['nonLocking'] ) ) { // Try to lock those files for the scope of this function... @@ -138,46 +136,29 @@ class FileBackendMultiWrite extends FileBackend { return $status; // abort } } - // Clear any cache entries (after locks acquired) $this->clearCache(); - // Do a consistency check to see if the backends agree - if ( count( $this->backends ) > 1 ) { - $status->merge( $this->consistencyCheck( array_merge( $paths['sh'], $paths['ex'] ) ) ); - if ( !$status->isOK() ) { - return $status; // abort + $status->merge( $this->consistencyCheck( array_merge( $paths['sh'], $paths['ex'] ) ) ); + if ( !$status->isOK() ) { + return $status; // abort + } + // Actually attempt the operation batch on the master backend... + $masterStatus = $mbe->doOperations( $realOps, $opts ); + $status->merge( $masterStatus ); + // Propagate the operations to the clone backends... + foreach ( $this->backends as $index => $backend ) { + if ( $index !== $this->masterIndex ) { // not done already + $realOps = $this->substOpBatchPaths( $ops, $backend ); + $status->merge( $backend->doOperations( $realOps, $opts ) ); } } - - // Actually attempt the operation batch... - $subStatus = FileOpBatch::attempt( $performOps, $opts, $this->fileJournal ); - - $success = array(); - $failCount = 0; - $successCount = 0; // Make 'success', 'successCount', and 'failCount' fields reflect // the overall operation, rather than all the batches for each backend. // Do this by only using success values from the master backend's batch. - $batchStart = $this->masterIndex * count( $ops ); - $batchEnd = $batchStart + count( $ops ) - 1; - for ( $i = $batchStart; $i <= $batchEnd; $i++ ) { - if ( !isset( $subStatus->success[$i] ) ) { - break; // failed out before trying this op - } elseif ( $subStatus->success[$i] ) { - ++$successCount; - } else { - ++$failCount; - } - $success[] = $subStatus->success[$i]; - } - $subStatus->success = $success; - $subStatus->successCount = $successCount; - $subStatus->failCount = $failCount; - - // Merge errors into status fields - $status->merge( $subStatus ); - $status->success = $subStatus->success; // not done in merge() + $status->success = $masterStatus->success; + $status->successCount = $masterStatus->successCount; + $status->failCount = $masterStatus->failCount; return $status; } @@ -190,21 +171,29 @@ class FileBackendMultiWrite extends FileBackend { */ public function consistencyCheck( array $paths ) { $status = Status::newGood(); - if ( $this->syncChecks == 0 ) { + if ( $this->syncChecks == 0 || count( $this->backends ) <= 1 ) { return $status; // skip checks } $mBackend = $this->backends[$this->masterIndex]; foreach ( array_unique( $paths ) as $path ) { $params = array( 'src' => $path, 'latest' => true ); + $mParams = $this->substOpPaths( $params, $mBackend ); // Stat the file on the 'master' backend - $mStat = $mBackend->getFileStat( $this->substOpPaths( $params, $mBackend ) ); + $mStat = $mBackend->getFileStat( $mParams ); + if ( $this->syncChecks & self::CHECK_SHA1 ) { + $mSha1 = $mBackend->getFileSha1( $mParams ); + } else { + $mSha1 = false; + } + $mUsable = $mBackend->isPathUsableInternal( $mParams['src'] ); // Check of all clone backends agree with the master... foreach ( $this->backends as $index => $cBackend ) { if ( $index === $this->masterIndex ) { continue; // master } - $cStat = $cBackend->getFileStat( $this->substOpPaths( $params, $cBackend ) ); + $cParams = $this->substOpPaths( $params, $cBackend ); + $cStat = $cBackend->getFileStat( $cParams ); if ( $mStat ) { // file is in master if ( !$cStat ) { // file should exist $status->fatal( 'backend-fail-synced', $path ); @@ -224,11 +213,20 @@ class FileBackendMultiWrite extends FileBackend { continue; } } + if ( $this->syncChecks & self::CHECK_SHA1 ) { + if ( $cBackend->getFileSha1( $cParams ) !== $mSha1 ) { // wrong SHA1 + $status->fatal( 'backend-fail-synced', $path ); + continue; + } + } } else { // file is not in master if ( $cStat ) { // file should not exist $status->fatal( 'backend-fail-synced', $path ); } } + if ( $mUsable !== $cBackend->isPathUsableInternal( $cParams['src'] ) ) { + $status->fatal( 'backend-fail-synced', $path ); + } } } @@ -302,10 +300,12 @@ class FileBackendMultiWrite extends FileBackend { * @see FileBackend::doQuickOperationsInternal() * @return Status */ - public function doQuickOperationsInternal( array $ops ) { + protected function doQuickOperationsInternal( array $ops ) { + $status = Status::newGood(); // Do the operations on the master backend; setting Status fields... $realOps = $this->substOpBatchPaths( $ops, $this->backends[$this->masterIndex] ); - $status = $this->backends[$this->masterIndex]->doQuickOperations( $realOps ); + $masterStatus = $this->backends[$this->masterIndex]->doQuickOperations( $realOps ); + $status->merge( $masterStatus ); // Propagate the operations to the clone backends... foreach ( $this->backends as $index => $backend ) { if ( $index !== $this->masterIndex ) { // not done already @@ -313,6 +313,12 @@ class FileBackendMultiWrite extends FileBackend { $status->merge( $backend->doQuickOperations( $realOps ) ); } } + // Make 'success', 'successCount', and 'failCount' fields reflect + // the overall operation, rather than all the batches for each backend. + // Do this by only using success values from the master backend's batch. + $status->success = $masterStatus->success; + $status->successCount = $masterStatus->successCount; + $status->failCount = $masterStatus->failCount; return $status; } diff --git a/includes/filerepo/backend/FileBackendStore.php b/includes/filerepo/backend/FileBackendStore.php index 49bb039497..4a9cff2eea 100644 --- a/includes/filerepo/backend/FileBackendStore.php +++ b/includes/filerepo/backend/FileBackendStore.php @@ -966,7 +966,7 @@ abstract class FileBackendStore extends FileBackend { * @see FileBackend::doOperationsInternal() * @return Status */ - protected function doOperationsInternal( array $ops, array $opts ) { + final protected function doOperationsInternal( array $ops, array $opts ) { wfProfileIn( __METHOD__ ); wfProfileIn( __METHOD__ . '-' . $this->name ); $status = Status::newGood(); diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index 61507f5480..6fb7acef75 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -243,6 +243,8 @@ class FileBackendTest extends MediaWikiTestCase { $props2 = $this->backend->getFileProps( array( 'src' => $dest ) ); $this->assertEquals( $props1, $props2, "Source and destination have the same props ($backendName)." ); + + $this->assertBackendPathsConsistent( array( $dest ) ); } public function provider_testStore() { @@ -330,6 +332,8 @@ class FileBackendTest extends MediaWikiTestCase { $props2 = $this->backend->getFileProps( array( 'src' => $dest ) ); $this->assertEquals( $props1, $props2, "Source and destination have the same props ($backendName)." ); + + $this->assertBackendPathsConsistent( array( $source, $dest ) ); } public function provider_testCopy() { @@ -419,6 +423,8 @@ class FileBackendTest extends MediaWikiTestCase { "Source file does not exist accourding to props ($backendName)." ); $this->assertEquals( true, $props2['fileExists'], "Destination file exists accourding to props ($backendName)." ); + + $this->assertBackendPathsConsistent( array( $source, $dest ) ); } public function provider_testMove() { @@ -504,6 +510,8 @@ class FileBackendTest extends MediaWikiTestCase { $props1 = $this->backend->getFileProps( array( 'src' => $source ) ); $this->assertFalse( $props1['fileExists'], "Source file $source does not exist according to props ($backendName)." ); + + $this->assertBackendPathsConsistent( array( $source ) ); } public function provider_testDelete() { @@ -595,6 +603,8 @@ class FileBackendTest extends MediaWikiTestCase { $this->backend->getFileSize( array( 'src' => $dest ) ), "Destination file $dest has original size according to props ($backendName)." ); } + + $this->assertBackendPathsConsistent( array( $dest ) ); } /** @@ -1822,6 +1832,13 @@ class FileBackendTest extends MediaWikiTestCase { $this->backend->clean( array( 'dir' => "$base/$container", 'recursive' => 1 ) ); } + function assertBackendPathsConsistent( array $paths ) { + if ( $this->backend instanceof FileBackendMultiWrite ) { + $status = $this->backend->consistencyCheck( $paths ); + $this->assertGoodStatus( $status, "Files synced: " . implode( ',', $paths ) ); + } + } + function assertGoodStatus( $status, $msg ) { $this->assertEquals( print_r( array(), 1 ), print_r( $status->errors, 1 ), $msg ); } -- 2.20.1