r108355 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108354‎ | r108355 | r108356 >
Date:09:25, 8 January 2012
Author:aaron
Status:ok
Tags:
Comment:
* Follow-up r107170: Moved FileBackend::concatenate() outside of doOperations() as it's own separate operation. It does not mutate storage files like the others and having it in doOperations() broke FileBackendMultiWrite. This change also makes overriding doOperationsInternal() easier (suching as using a custom batch operation storage API).
* Added sanity check to FileBackendMultiWrite constructor.
* Moved FileBackend::create() function up a bit.
Modified paths:
  • /trunk/phase3/includes/filerepo/FileRepo.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileOp.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -437,13 +437,12 @@
438438 $this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus );
439439 $this->tearDownFiles();
440440
441 - # FIXME
442 - #$this->backend = $this->multiBackend;
443 - #$this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus );
444 - #$this->tearDownFiles();
 441+ $this->backend = $this->multiBackend;
 442+ $this->doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus );
 443+ $this->tearDownFiles();
445444 }
446445
447 - public function doTestConcatenate( $op, $srcs, $srcsContent, $alreadyExists, $okStatus ) {
 446+ public function doTestConcatenate( $params, $srcs, $srcsContent, $alreadyExists, $okStatus ) {
448447 $backendName = $this->backendClass();
449448
450449 $expContent = '';
——@@ -462,7 +461,7 @@
463462 $this->assertEquals( true, $status->isOK(),
464463 "Creation of source files succeeded ($backendName)." );
465464
466 - $dest = $op['dst'];
 465+ $dest = $params['dst'];
467466 if ( $alreadyExists ) {
468467 $ok = file_put_contents( $dest, 'blah...blah...waahwaah' ) !== false;
469468 $this->assertEquals( true, $ok,
——@@ -473,8 +472,8 @@
474473 "Creation of 0-byte file at $dest succeeded ($backendName)." );
475474 }
476475
477 - // Combine them
478 - $status = $this->backend->doOperation( $op );
 476+ // Combine the files into one
 477+ $status = $this->backend->concatenate( $params );
479478 if ( $okStatus ) {
480479 $this->assertEquals( array(), $status->errors,
481480 "Creation of concat file at $dest succeeded without warnings ($backendName)." );
——@@ -534,10 +533,10 @@
535534 'lkaem;a',
536535 'legma'
537536 );
538 - $op = array( 'op' => 'concatenate', 'srcs' => $srcs, 'dst' => $dest );
 537+ $params = array( 'srcs' => $srcs, 'dst' => $dest );
539538
540539 $cases[] = array(
541 - $op, // operation
 540+ $params, // operation
542541 $srcs, // sources
543542 $content, // content for each source
544543 false, // no dest already exists
——@@ -545,7 +544,7 @@
546545 );
547546
548547 $cases[] = array(
549 - $op, // operation
 548+ $params, // operation
550549 $srcs, // sources
551550 $content, // content for each source
552551 true, // dest already exists
Index: trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
——@@ -36,9 +36,15 @@
3737 */
3838 public function __construct( array $config ) {
3939 parent::__construct( $config );
 40+ $namesUsed = array();
4041 // Construct backends here rather than via registration
4142 // to keep these backends hidden from outside the proxy.
4243 foreach ( $config['backends'] as $index => $config ) {
 44+ $name = $config['name'];
 45+ if ( isset( $namesUsed[$name] ) ) { // don't break FileOp predicates
 46+ throw new MWException( "Two or more backends defined with the name $name." );
 47+ }
 48+ $namesUsed[$name] = 1;
4349 if ( !isset( $config['class'] ) ) {
4450 throw new MWException( 'No class given for a backend config.' );
4551 }
——@@ -245,6 +251,15 @@
246252 }
247253
248254 /**
 255+ * @see FileBackendBase::getFileList()
 256+ */
 257+ public function concatenate( array $params ) {
 258+ // We are writing to an FS file, so we don't need to do this per-backend
 259+ $realParams = $this->substOpPaths( $params, $this->backends[$this->masterIndex] );
 260+ return $this->backends[$this->masterIndex]->concatenate( $realParams );
 261+ }
 262+
 263+ /**
249264 * @see FileBackendBase::fileExists()
250265 */
251266 public function fileExists( array $params ) {
Index: trunk/phase3/includes/filerepo/backend/FileOp.php
——@@ -808,61 +808,6 @@
809809 }
810810
811811 /**
812 - * Combines files from severals storage paths into a new file in the backend.
813 - * Parameters similar to FileBackend::concatenate(), which include:
814 - * srcs : ordered source storage paths (e.g. chunk1, chunk2, ...)
815 - * dst : destination file system path to 0-byte temp file
816 - */
817 -class ConcatenateFileOp extends FileOp {
818 - protected function allowedParams() {
819 - return array( 'srcs', 'dst' );
820 - }
821 -
822 - protected function doPrecheck( array &$predicates ) {
823 - $status = Status::newGood();
824 - // Check destination temp file
825 - wfSuppressWarnings();
826 - $ok = ( is_file( $this->params['dst'] ) && !filesize( $this->params['dst'] ) );
827 - wfRestoreWarnings();
828 - if ( !$ok ) { // not present or not empty
829 - $status->fatal( 'backend-fail-opentemp', $this->params['dst'] );
830 - return $status;
831 - }
832 - // Check that source files exists
833 - foreach ( $this->params['srcs'] as $source ) {
834 - if ( !$this->fileExists( $source, $predicates ) ) {
835 - $status->fatal( 'backend-fail-notexists', $source );
836 - return $status;
837 - }
838 - }
839 - return $status;
840 - }
841 -
842 - protected function doAttempt() {
843 - $status = Status::newGood();
844 - // Concatenate the file at the destination
845 - $status->merge( $this->backend->concatenateInternal( $this->params ) );
846 - return $status;
847 - }
848 -
849 - protected function doRevert() {
850 - $status = Status::newGood();
851 - // Clear out the temp file back to 0-bytes
852 - wfSuppressWarnings();
853 - $ok = file_put_contents( $this->params['dst'], '' );
854 - wfRestoreWarnings();
855 - if ( !$ok ) {
856 - $status->fatal( 'backend-fail-writetemp', $this->params['dst'] );
857 - }
858 - return $status;
859 - }
860 -
861 - public function storagePathsRead() {
862 - return $this->params['srcs'];
863 - }
864 -}
865 -
866 -/**
867812 * Delete a file at the storage path.
868813 * Parameters similar to FileBackend::delete(), which include:
869814 * src : source storage path
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -132,14 +132,8 @@
133133 * 'src' => <storage path>,
134134 * 'ignoreMissingSource' => <boolean>
135135 * )
136 - * f) Concatenate a list of files within storage into a single temp file
 136+ * f) Do nothing (no-op)
137137 * array(
138 - * 'op' => 'concatenate',
139 - * 'srcs' => <ordered array of storage paths>,
140 - * 'dst' => <file system path to 0-byte temp file>
141 - * )
142 - * g) Do nothing (no-op)
143 - * array(
144138 * 'op' => 'null',
145139 * )
146140 *
——@@ -203,6 +197,21 @@
204198 }
205199
206200 /**
 201+ * Performs a single create operation.
 202+ * This sets $params['op'] to 'create' and passes it to doOperation().
 203+ *
 204+ * @see FileBackendBase::doOperation()
 205+ *
 206+ * @param $params Array Operation parameters
 207+ * @param $opts Array Operation options
 208+ * @return Status
 209+ */
 210+ final public function create( array $params, array $opts = array() ) {
 211+ $params['op'] = 'create';
 212+ return $this->doOperation( $params, $opts );
 213+ }
 214+
 215+ /**
207216 * Performs a single store operation.
208217 * This sets $params['op'] to 'store' and passes it to doOperation().
209218 *
——@@ -263,36 +272,17 @@
264273 }
265274
266275 /**
267 - * Performs a single create operation.
268 - * This sets $params['op'] to 'create' and passes it to doOperation().
 276+ * Concatenate a list of storage files into a single file on the file system
 277+ * $params include:
 278+ * srcs : ordered source storage paths (e.g. chunk1, chunk2, ...)
 279+ * dst : file system path to 0-byte temp file
269280 *
270 - * @see FileBackendBase::doOperation()
271 - *
272281 * @param $params Array Operation parameters
273 - * @param $opts Array Operation options
274282 * @return Status
275283 */
276 - final public function create( array $params, array $opts = array() ) {
277 - $params['op'] = 'create';
278 - return $this->doOperation( $params, $opts );
279 - }
 284+ abstract public function concatenate( array $params );
280285
281286 /**
282 - * Performs a single concatenate operation.
283 - * This sets $params['op'] to 'concatenate' and passes it to doOperation().
284 - *
285 - * @see FileBackendBase::doOperation()
286 - *
287 - * @param $params Array Operation parameters
288 - * @param $opts Array Operation options
289 - * @return Status
290 - */
291 - final public function concatenate( array $params, array $opts = array() ) {
292 - $params['op'] = 'concatenate';
293 - return $this->doOperation( $params, $opts );
294 - }
295 -
296 - /**
297287 * Prepare a storage path for usage. This will create containers
298288 * that don't yet exist or, on FS backends, create parent directories.
299289 *
——@@ -677,24 +667,27 @@
678668 }
679669
680670 /**
681 - * Combines files from several storage paths into a new file in the backend.
682 - * Do not call this function from places outside FileBackend and FileOp.
683 - * $params include:
684 - * srcs : ordered source storage paths (e.g. chunk1, chunk2, ...)
685 - * dst : file system path to 0-byte temp file
686 - *
687 - * @param $params Array
688 - * @return Status
 671+ * @see FileBackendBase::concatenate()
689672 */
690 - final public function concatenateInternal( array $params ) {
691 - $status = $this->doConcatenateInternal( $params );
 673+ final public function concatenate( array $params ) {
 674+ $status = Status::newGood();
 675+
 676+ // Try to lock the source files for the scope of this function
 677+ $scopeLockS = $this->getScopedFileLocks( $params['srcs'], LockManager::LOCK_UW, $status );
 678+ if ( !$status->isOK() ) {
 679+ return $status; // abort
 680+ }
 681+
 682+ // Actually do the concatenation
 683+ $status->merge( $this->doConcatenate( $params ) );
 684+
692685 return $status;
693686 }
694687
695688 /**
696 - * @see FileBackend::concatenateInternal()
 689+ * @see FileBackend::concatenate()
697690 */
698 - protected function doConcatenateInternal( array $params ) {
 691+ protected function doConcatenate( array $params ) {
699692 $status = Status::newGood();
700693 $tmpPath = $params['dst']; // convenience
701694
——@@ -1035,7 +1028,6 @@
10361029 'copy' => 'CopyFileOp',
10371030 'move' => 'MoveFileOp',
10381031 'delete' => 'DeleteFileOp',
1039 - 'concatenate' => 'ConcatenateFileOp',
10401032 'create' => 'CreateFileOp',
10411033 'null' => 'NullFileOp'
10421034 );
Index: trunk/phase3/includes/filerepo/FileRepo.php
——@@ -791,8 +791,6 @@
792792 */
793793 function concatenate( $srcPaths, $dstPath, $flags = 0 ) {
794794 $status = $this->newGood();
795 - // Resolve target to a storage path if virtual
796 - $dest = $this->resolveToStoragePath( $dstPath );
797795
798796 $sources = array();
799797 $deleteOperations = array(); // post-concatenate ops
——@@ -805,9 +803,9 @@
806804 }
807805 }
808806
809 - // Concatenate the chunks into one file
810 - $op = array( 'op' => 'concatenate', 'srcs' => $sources, 'dst' => $dest );
811 - $status->merge( $this->backend->doOperation( $op ) );
 807+ // Concatenate the chunks into one FS file
 808+ $params = array( 'srcs' => $sources, 'dst' => $dstPath );
 809+ $status->merge( $this->backend->concatenate( $params ) );
812810 if ( !$status->isOK() ) {
813811 return $status;
814812 }

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r107170In FileBackendBase/FileBackend:...aaron18:59, 23 December 2011

Status & tagging log