From dcb81eef85975a4e836def27914a72cc3eca7860 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 22 Sep 2012 11:46:48 -0700 Subject: [PATCH] [FileBackend] Optimized concatenate() to use getLocalReferenceMulti(). Change-Id: I884eb3fc27adb48ec6761143190cc622f1de2dca --- includes/filebackend/FileBackend.php | 5 ++- includes/filebackend/FileBackendStore.php | 30 ++++++++----- .../includes/filerepo/FileBackendTest.php | 44 ++++++++++++------- 3 files changed, 51 insertions(+), 28 deletions(-) diff --git a/includes/filebackend/FileBackend.php b/includes/filebackend/FileBackend.php index 364f2183fc..eaf91036eb 100644 --- a/includes/filebackend/FileBackend.php +++ b/includes/filebackend/FileBackend.php @@ -573,8 +573,9 @@ abstract class FileBackend { * * @param $params Array Operation parameters * $params include: - * - srcs : ordered source storage paths (e.g. chunk1, chunk2, ...) - * - dst : file system path to 0-byte temp file + * - srcs : ordered source storage paths (e.g. chunk1, chunk2, ...) + * - dst : file system path to 0-byte temp file + * - parallelize : try to do operations in parallel when possible * @return Status */ abstract public function concatenate( array $params ); diff --git a/includes/filebackend/FileBackendStore.php b/includes/filebackend/FileBackendStore.php index 112f247614..4271c4a814 100644 --- a/includes/filebackend/FileBackendStore.php +++ b/includes/filebackend/FileBackendStore.php @@ -314,31 +314,41 @@ abstract class FileBackendStore extends FileBackend { protected function doConcatenate( array $params ) { $status = Status::newGood(); $tmpPath = $params['dst']; // convenience + unset( $params['latest'] ); // sanity // Check that the specified temp file is valid... wfSuppressWarnings(); - $ok = ( is_file( $tmpPath ) && !filesize( $tmpPath ) ); + $ok = ( is_file( $tmpPath ) && filesize( $tmpPath ) == 0 ); wfRestoreWarnings(); if ( !$ok ) { // not present or not empty $status->fatal( 'backend-fail-opentemp', $tmpPath ); return $status; } - // Build up the temp file using the source chunks (in order)... + // Get local FS versions of the chunks needed for the concatenation... + $fsFiles = $this->getLocalReferenceMulti( $params ); + foreach ( $fsFiles as $path => &$fsFile ) { + if ( !$fsFile ) { // chunk failed to download? + $fsFile = $this->getLocalReference( array( 'src' => $path ) ); + if ( !$fsFile ) { // retry failed? + $status->fatal( 'backend-fail-read', $path ); + return $status; + } + } + } + unset( $fsFile ); // unset reference so we can reuse $fsFile + + // Get a handle for the destination temp file $tmpHandle = fopen( $tmpPath, 'ab' ); if ( $tmpHandle === false ) { $status->fatal( 'backend-fail-opentemp', $tmpPath ); return $status; } - foreach ( $params['srcs'] as $virtualSource ) { - // Get a local FS version of the chunk - $tmpFile = $this->getLocalReference( array( 'src' => $virtualSource ) ); - if ( !$tmpFile ) { - $status->fatal( 'backend-fail-read', $virtualSource ); - return $status; - } + + // Build up the temp file using the source chunks (in order)... + foreach ( $fsFiles as $virtualSource => $fsFile ) { // Get a handle to the local FS version - $sourceHandle = fopen( $tmpFile->getPath(), 'rb' ); + $sourceHandle = fopen( $fsFile->getPath(), 'rb' ); if ( $sourceHandle === false ) { fclose( $tmpHandle ); $status->fatal( 'backend-fail-read', $virtualSource ); diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php index 2de569b584..fa2afaa6bf 100644 --- a/tests/phpunit/includes/filerepo/FileBackendTest.php +++ b/tests/phpunit/includes/filerepo/FileBackendTest.php @@ -938,10 +938,11 @@ class FileBackendTest extends MediaWikiTestCase { $backendName = $this->backendClass(); $srcs = (array)$source; - foreach ( $srcs as $src ) { + $content = (array)$content; + foreach ( $srcs as $i => $src ) { $this->prepare( array( 'dir' => dirname( $src ) ) ); $status = $this->backend->doOperation( - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) ); + array( 'op' => 'create', 'content' => $content[$i], 'dst' => $src ) ); $this->assertGoodStatus( $status, "Creation of file at $src succeeded ($backendName)." ); } @@ -950,13 +951,15 @@ class FileBackendTest extends MediaWikiTestCase { $contents = $this->backend->getFileContentsMulti( array( 'srcs' => $source ) ); foreach ( $contents as $path => $data ) { $this->assertNotEquals( false, $data, "Contents of $path exists ($backendName)." ); - $this->assertEquals( $content, $data, "Contents of $path is correct ($backendName)." ); + $this->assertEquals( current( $content ), $data, "Contents of $path is correct ($backendName)." ); + next( $content ); } $this->assertEquals( $source, array_keys( $contents ), "Contents in right order ($backendName)." ); + $this->assertEquals( count( $source ), count( $contents ), "Contents array size correct ($backendName)." ); } else { $data = $this->backend->getFileContents( array( 'src' => $source ) ); $this->assertNotEquals( false, $data, "Contents of $source exists ($backendName)." ); - $this->assertEquals( $content, $data, "Contents of $source is correct ($backendName)." ); + $this->assertEquals( $content[0], $data, "Contents of $source is correct ($backendName)." ); } } @@ -968,8 +971,9 @@ class FileBackendTest extends MediaWikiTestCase { $cases[] = array( "$base/unittest-cont1/e/b/some-other_file.txt", "more file contents" ); $cases[] = array( array( "$base/unittest-cont1/e/a/x.txt", "$base/unittest-cont1/e/a/y.txt", - "$base/unittest-cont1/e/a/z.txt" ), - "some clone file contents" ); + "$base/unittest-cont1/e/a/z.txt" ), + array( "contents xx", "contents xy", "contents xz" ) + ); return $cases; } @@ -993,10 +997,11 @@ class FileBackendTest extends MediaWikiTestCase { $backendName = $this->backendClass(); $srcs = (array)$source; - foreach ( $srcs as $src ) { + $content = (array)$content; + foreach ( $srcs as $i => $src ) { $this->prepare( array( 'dir' => dirname( $src ) ) ); $status = $this->backend->doOperation( - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) ); + array( 'op' => 'create', 'content' => $content[$i], 'dst' => $src ) ); $this->assertGoodStatus( $status, "Creation of file at $src succeeded ($backendName)." ); } @@ -1008,16 +1013,18 @@ class FileBackendTest extends MediaWikiTestCase { "Creation of local copy of $path succeeded ($backendName)." ); $contents = file_get_contents( $tmpFile->getPath() ); $this->assertNotEquals( false, $contents, "Local copy of $path exists ($backendName)." ); - $this->assertEquals( $content, $contents, "Local copy of $path is correct ($backendName)." ); + $this->assertEquals( current( $content ), $contents, "Local copy of $path is correct ($backendName)." ); + next( $content ); } $this->assertEquals( $source, array_keys( $tmpFiles ), "Local copies in right order ($backendName)." ); + $this->assertEquals( count( $source ), count( $tmpFiles ), "Local copies array size correct ($backendName)." ); } else { $tmpFile = $this->backend->getLocalCopy( array( 'src' => $source ) ); $this->assertNotNull( $tmpFile, "Creation of local copy of $source succeeded ($backendName)." ); $contents = file_get_contents( $tmpFile->getPath() ); $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." ); - $this->assertEquals( $content, $contents, "Local copy of $source is correct ($backendName)." ); + $this->assertEquals( $content[0], $contents, "Local copy of $source is correct ($backendName)." ); } } @@ -1031,7 +1038,8 @@ class FileBackendTest extends MediaWikiTestCase { $cases[] = array( array( "$base/unittest-cont1/e/a/x.txt", "$base/unittest-cont1/e/a/y.txt", "$base/unittest-cont1/e/a/z.txt" ), - "clone file contents" ); + array( "contents xx", "contents xy", "contents xz" ) + ); return $cases; } @@ -1055,10 +1063,11 @@ class FileBackendTest extends MediaWikiTestCase { $backendName = $this->backendClass(); $srcs = (array)$source; - foreach ( $srcs as $src ) { + $content = (array)$content; + foreach ( $srcs as $i => $src ) { $this->prepare( array( 'dir' => dirname( $src ) ) ); $status = $this->backend->doOperation( - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) ); + array( 'op' => 'create', 'content' => $content[$i], 'dst' => $src ) ); $this->assertGoodStatus( $status, "Creation of file at $src succeeded ($backendName)." ); } @@ -1070,16 +1079,18 @@ class FileBackendTest extends MediaWikiTestCase { "Creation of local copy of $path succeeded ($backendName)." ); $contents = file_get_contents( $tmpFile->getPath() ); $this->assertNotEquals( false, $contents, "Local ref of $path exists ($backendName)." ); - $this->assertEquals( $content, $contents, "Local ref of $path is correct ($backendName)." ); + $this->assertEquals( current( $content ), $contents, "Local ref of $path is correct ($backendName)." ); + next( $content ); } $this->assertEquals( $source, array_keys( $tmpFiles ), "Local refs in right order ($backendName)." ); + $this->assertEquals( count( $source ), count( $tmpFiles ), "Local refs array size correct ($backendName)." ); } else { $tmpFile = $this->backend->getLocalReference( array( 'src' => $source ) ); $this->assertNotNull( $tmpFile, "Creation of local copy of $source succeeded ($backendName)." ); $contents = file_get_contents( $tmpFile->getPath() ); $this->assertNotEquals( false, $contents, "Local ref of $source exists ($backendName)." ); - $this->assertEquals( $content, $contents, "Local ref of $source is correct ($backendName)." ); + $this->assertEquals( $content[0], $contents, "Local ref of $source is correct ($backendName)." ); } } @@ -1093,7 +1104,8 @@ class FileBackendTest extends MediaWikiTestCase { $cases[] = array( array( "$base/unittest-cont1/e/a/x.txt", "$base/unittest-cont1/e/a/y.txt", "$base/unittest-cont1/e/a/z.txt" ), - "clone file contents" ); + array( "contents xx", "contents xy", "contents xz" ) + ); return $cases; } -- 2.20.1