From 50a7097436a55d48a70ca83bb0c7faac5148bb0f Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 8 Jan 2012 22:10:53 +0000 Subject: [PATCH] * Fixed 'success' value of doOperations() Status to match documentation. * Made 'success', 'successCount', and 'failCount' fields reflect the overall operation in FileBackendMultiWrite::doOperationsInternal(). This makes it match up with single-write backends. * Made FileBackend::clearCache() part of the public API. --- includes/filerepo/backend/FileBackend.php | 21 +++-- .../backend/FileBackendMultiWrite.php | 88 ++++++++++++++----- .../includes/filerepo/FileBackendTest.php | 10 +++ 3 files changed, 93 insertions(+), 26 deletions(-) diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 613447dcae..5befbe0c38 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -479,6 +479,15 @@ abstract class FileBackendBase { */ abstract public function getFileList( array $params ); + /** + * Invalidate any in-process file existence and property cache. + * If $paths is given, then only the cache for those files will be cleared. + * + * @param $paths Array Storage paths + * @return void + */ + abstract public function clearCache( array $paths = null ); + /** * Lock the files at the given storage paths in the backend. * This will either lock all the files or none (on failure). @@ -1094,17 +1103,19 @@ abstract class FileBackend extends FileBackendBase { // Clear any cache entries (after locks acquired) $this->clearCache(); + // Actually attempt the operation batch... - $status->merge( FileOp::attemptBatch( $performOps, $opts ) ); + $subStatus = FileOp::attemptBatch( $performOps, $opts ); + + // Merge errors into status fields + $status->merge( $subStatus ); + $status->success = $subStatus->success; // not done in merge() return $status; } /** - * Invalidate the file existence and property cache - * - * @param $paths Array Clear cache for specific files - * @return void + * @see FileBackendBase::clearCache() */ final public function clearCache( array $paths = null ) { if ( $paths === null ) { diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php index 10ebd7e456..b71b631030 100644 --- a/includes/filerepo/backend/FileBackendMultiWrite.php +++ b/includes/filerepo/backend/FileBackendMultiWrite.php @@ -14,7 +14,7 @@ * Only use this class when transitioning from one storage system to another. * * Read operations are only done on the 'master' backend for consistency. - * All write operations are performed on all backends, in the order defined. + * Write operations are performed on all backends, in the order defined. * If an operation fails on one backend it will be rolled back from the others. * * @ingroup FileBackend @@ -86,8 +86,8 @@ class FileBackendMultiWrite extends FileBackendBase { $filesChanged = array_merge( $filesChanged, $fileOp->storagePathsChanged() ); } // Get the paths under the proxy backend's name - $this->unsubstPaths( $filesRead ); - $this->unsubstPaths( $filesChanged ); + $filesRead = $this->unsubstPaths( $filesRead ); + $filesChanged = $this->unsubstPaths( $filesChanged ); } } @@ -103,9 +103,7 @@ class FileBackendMultiWrite extends FileBackendBase { } // Clear any cache entries (after locks acquired) - foreach ( $this->backends as $backend ) { - $backend->clearCache(); - } + $this->clearCache(); // Do a consistency check to see if the backends agree if ( count( $this->backends ) > 1 ) { @@ -116,7 +114,32 @@ class FileBackendMultiWrite extends FileBackendBase { } // Actually attempt the operation batch... - $status->merge( FileOp::attemptBatch( $performOps, $opts ) ); + $subStatus = FileOp::attemptBatch( $performOps, $opts ); + + $success = array(); + $failCount = $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() return $status; } @@ -166,7 +189,7 @@ class FileBackendMultiWrite extends FileBackendBase { /** * Substitute the backend name in storage path parameters - * for a set of operations with that of a given backend. + * for a set of operations with that of a given internal backend. * * @param $ops Array List of file operation arrays * @param $backend FileBackend @@ -177,12 +200,8 @@ class FileBackendMultiWrite extends FileBackendBase { foreach ( $ops as $op ) { $newOp = $op; // operation foreach ( array( 'src', 'srcs', 'dst', 'dir' ) as $par ) { - if ( isset( $newOp[$par] ) ) { - $newOp[$par] = preg_replace( - '!^mwstore://' . preg_quote( $this->name ) . '/!', - 'mwstore://' . $backend->getName() . '/', - $newOp[$par] // string or array - ); + if ( isset( $newOp[$par] ) ) { // string or array + $newOp[$par] = $this->substPaths( $newOp[$par], $backend ); } } $newOps[] = $newOp; @@ -203,15 +222,32 @@ class FileBackendMultiWrite extends FileBackendBase { } /** - * Replace the backend part of storage paths with this backend's name + * Substitute the backend of storage paths with an internal backend's name * - * @param &$paths Array - * @return void + * @param $paths Array|string List of paths or single string path + * @param $backend FileBackend + * @return Array|string */ - protected function unsubstPaths( array &$paths ) { - foreach ( $paths as &$path ) { - $path = preg_replace( '!^mwstore://([^/]+)!', "mwstore://{$this->name}", $path ); - } + protected function substPaths( $paths, FileBackend $backend ) { + return preg_replace( + '!^mwstore://' . preg_quote( $this->name ) . '/!', + 'mwstore://' . $backend->getName() . '/', + $paths // string or array + ); + } + + /** + * Substitute the backend of internal storage paths with the proxy backend's name + * + * @param $paths Array|string List of paths or single string path + * @return Array|string + */ + protected function unsubstPaths( $paths ) { + return preg_replace( + '!^mwstore://([^/]+)!', + "mwstore://{$this->name}", + $paths // string or array + ); } /** @@ -346,4 +382,14 @@ class FileBackendMultiWrite extends FileBackendBase { $realParams = $this->substOpPaths( $params, $this->backends[$this->masterIndex] ); return $this->backends[$this->masterIndex]->getFileList( $realParams ); } + + /** + * @see FileBackendBase::clearCache() + */ + public function clearCache( array $paths = null ) { + foreach ( $this->backends as $backend ) { + $realPaths = is_array( $paths ) ? $this->substPaths( $paths ) : null; + $backend->clearCache( $realPaths ); + } + } } diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index aef77291c1..c9f5f3a90a 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -76,6 +76,8 @@ class FileBackendTest extends MediaWikiTestCase { "Store from $source to $dest succeeded without warnings ($backendName)." ); $this->assertEquals( true, $status->isOK(), "Store from $source to $dest succeeded ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Store from $source to $dest has proper 'success' field in Status ($backendName)." ); $this->assertEquals( true, file_exists( $source ), "Source file $source still exists ($backendName)." ); $this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ), @@ -142,6 +144,8 @@ class FileBackendTest extends MediaWikiTestCase { "Copy from $source to $dest succeeded without warnings ($backendName)." ); $this->assertEquals( true, $status->isOK(), "Copy from $source to $dest succeeded ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Copy from $source to $dest has proper 'success' field in Status ($backendName)." ); $this->assertEquals( true, $this->backend->fileExists( array( 'src' => $source ) ), "Source file $source still exists ($backendName)." ); $this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ), @@ -210,6 +214,8 @@ class FileBackendTest extends MediaWikiTestCase { "Move from $source to $dest succeeded without warnings ($backendName)." ); $this->assertEquals( true, $status->isOK(), "Move from $source to $dest succeeded ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Move from $source to $dest has proper 'success' field in Status ($backendName)." ); $this->assertEquals( false, $this->backend->fileExists( array( 'src' => $source ) ), "Source file $source does not still exists ($backendName)." ); $this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ), @@ -282,6 +288,8 @@ class FileBackendTest extends MediaWikiTestCase { "Deletion of file at $source succeeded without warnings ($backendName)." ); $this->assertEquals( true, $status->isOK(), "Deletion of file at $source succeeded ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Deletion of file at $source has proper 'success' field in Status ($backendName)." ); } else { $this->assertEquals( false, $status->isOK(), "Deletion of file at $source failed ($backendName)." ); @@ -362,6 +370,8 @@ class FileBackendTest extends MediaWikiTestCase { "Creation of file at $dest succeeded without warnings ($backendName)." ); $this->assertEquals( true, $status->isOK(), "Creation of file at $dest succeeded ($backendName)." ); + $this->assertEquals( array( 0 => true ), $status->success, + "Creation of file at $dest has proper 'success' field in Status ($backendName)." ); } else { $this->assertEquals( false, $status->isOK(), "Creation of file at $dest failed ($backendName)." ); -- 2.20.1