r106280 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r106279‎ | r106280 | r106281 >
Date:23:23, 14 December 2011
Author:aaron
Status:deferred (Comments)
Tags:
Comment:
* Made FileBackendMultiWrite substitute the backend name portion of storage paths as needed since each backend has it's own name (registration disallowes duplicate names).
* Refactored doOperations() code duplication into a FileBackendBase::attemptOperations() function. Removed code that reset $predicates from FileBackendMultiWrite as the paths include the backend name already, so there is no ambiguity.
* Made FileBackend::doOperations() use shared locks where possible (some lock managers may interpret LOCK_UW as LOCK_EX instead of LOCK_SH)
* Removed getFileHash() comment (function removed).
* Renamed FSFile::sha1Base36() -> FSFile::getSha1Base36()
Modified paths:
  • /branches/FileBackend/phase3/includes/filerepo/FileRepo.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileOp.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/FSFile.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/File.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/includes/filerepo/file/File.php
——@@ -1584,7 +1584,7 @@
15851585 wfDeprecated( __METHOD__, '1.19' );
15861586
15871587 $fsFile = new FSFile( $path );
1588 - return $fsFile->sha1Base36();
 1588+ return $fsFile->getSha1Base36();
15891589 }
15901590
15911591 /**
Index: branches/FileBackend/phase3/includes/filerepo/file/FSFile.php
——@@ -114,7 +114,7 @@
115115 $info = $this->extractImageSizeInfo( $gis ) + $info;
116116 }
117117 }
118 - $info['sha1'] = $this->sha1Base36();
 118+ $info['sha1'] = $this->getSha1Base36();
119119
120120 wfDebug(__METHOD__.": $this->path loaded, {$info['size']} bytes, {$info['mime']}.\n");
121121 } else {
——@@ -165,7 +165,7 @@
166166 *
167167 * @return false|string False on failure
168168 */
169 - public function sha1Base36() {
 169+ public function getSha1Base36() {
170170 wfProfileIn( __METHOD__ );
171171
172172 wfSuppressWarnings();
——@@ -206,6 +206,6 @@
207207 */
208208 static function getSha1Base36FromPath( $path ) {
209209 $fsFile = new self( $path );
210 - return $fsFile->sha1Base36();
 210+ return $fsFile->getSha1Base36();
211211 }
212212 }
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
——@@ -16,8 +16,6 @@
1717 * that has the file. Special cases are listed below:
1818 * a) getFileTimestamp() will always check only the master backend to
1919 * avoid confusing and inconsistent results.
20 - * b) getFileHash() will always check only the master backend to keep
21 - * the result format consistent.
2220 *
2321 * All write operations are performed on all backends.
2422 * If an operation fails on one backend it will be rolled back from the others.
——@@ -62,106 +60,87 @@
6361 }
6462
6563 final public function doOperations( array $ops ) {
66 - $status = Status::newGood( array() );
 64+ $status = Status::newGood();
6765
6866 $performOps = array(); // list of FileOp objects
69 - $batchSize = count( $ops ); // each backend has this many ops
70 - $filesToLock = array(); // storage paths to lock
 67+ $filesLockEx = $filesLockSh = array(); // storage paths to lock
7168 // Build up a list of FileOps. The list will have all the ops
7269 // for one backend, then all the ops for the next, and so on.
7370 // These batches of ops are all part of a continuous array.
7471 // Also build up a list of files to lock...
7572 foreach ( $this->fileBackends as $index => $backend ) {
76 - $performOps = array_merge( $performOps, $backend->getOperations( $ops ) );
 73+ $backendOps = $this->substOpPaths( $ops, $backend );
 74+ $performOps = array_merge( $performOps, $backend->getOperations( $backendOps ) );
7775 if ( $index == 0 ) {
78 - // Set $filesToLock from the first batch so we don't try to set all
 76+ // Set "files to lock" from the first batch so we don't try to set all
7977 // locks two or three times over (depending on the number of backends).
8078 // A lock on one storage path is a lock on all the backends.
8179 foreach ( $performOps as $index => $fileOp ) {
82 - $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsUsed() );
 80+ $filesLockSh = array_merge( $filesLockSh, $fileOp->storagePathsRead() );
 81+ $filesLockEx = array_merge( $filesLockEx, $fileOp->storagePathsChanged() );
8382 }
 83+ // Lock the paths under the proxy backend's name
 84+ $this->unsubstPaths( $filesLockSh );
 85+ $this->unsubstPaths( $filesLockEx );
8486 }
8587 }
8688
8789 // Try to lock those files for the scope of this function...
88 - $scopedLock = $this->getScopedFileLocks( $filesToLock, LockManager::LOCK_EX, $status );
 90+ $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
 91+ $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
8992 if ( !$status->isOK() ) {
9093 return $status; // abort
9194 }
9295
93 - $failedOps = array(); // failed ops with 'ignoreErrors'
94 - $predicates = FileOp::newPredicates(); // effects of previous ops
95 - // Do pre-checks for each operation; abort on failure...
96 - foreach ( $performOps as $index => $fileOp ) {
97 - if ( $index > 0 && ( $index % $batchSize ) == 0 ) {
98 - // We are starting the op batch for another backend
99 - // which is not effected by the other op batches.
100 - $predicates = FileOp::newPredicates();
101 - }
102 - $status->merge( $fileOp->precheck( $predicates ) );
103 - if ( !$status->isOK() ) { // operation failed?
104 - if ( $fileOp->getParam( 'ignoreErrors' ) ) {
105 - $failedOps[$index] = 1; // remember not to call attempt()/finish()
106 - ++$status->failCount;
107 - $status->success[$index] = false;
108 - } else {
109 - return $status;
110 - }
111 - }
112 - }
 96+ // Actually attempt the operation batch...
 97+ $status->merge( $this->attemptOperations( $performOps ) );
11398
114 - // Attempt each operation; abort on failure...
115 - foreach ( $performOps as $index => $fileOp ) {
116 - if ( isset( $failedOps[$index] ) ) {
117 - continue; // nothing to do
118 - }
119 - $status->merge( $fileOp->attempt() );
120 - if ( !$status->isOK() ) { // operation failed?
121 - if ( $fileOp->getParam( 'ignoreErrors' ) ) {
122 - $failedOps[$index] = 1; // remember not to call finish()
123 - ++$status->failCount;
124 - $status->success[$index] = false;
125 - } else {
126 - // Revert everything done so far and abort.
127 - // Do this by reverting each previous operation in reverse order.
128 - $pos = $index - 1; // last one failed; no need to revert()
129 - while ( $pos >= 0 ) {
130 - if ( !isset( $failedOps[$pos] ) ) {
131 - $status->merge( $performOps[$pos]->revert() );
132 - }
133 - $pos--;
134 - }
135 - return $status;
 99+ return $status;
 100+ }
 101+
 102+ /**
 103+ * Substitute the backend name in storage path parameters
 104+ * for a set of operations with a that of a given backend.
 105+ *
 106+ * @param $ops Array List of file operation arrays
 107+ * @param $backend FileBackend
 108+ * @return Array
 109+ */
 110+ protected function substOpPaths( array $ops, FileBackend $backend ) {
 111+ $newOps = array(); // operations
 112+ foreach ( $ops as $op ) {
 113+ $newOp = $op; // operation
 114+ foreach ( array( 'src', 'srcs', 'dst' ) as $par ) {
 115+ if ( isset( $newOp[$par] ) ) {
 116+ $newOp[$par] = preg_replace(
 117+ '!^mwstore://' . preg_quote( $this->name ) . '/!',
 118+ 'mwstore://' . $backend->getName() . '/',
 119+ $newOp[$par]
 120+ );
136121 }
137122 }
 123+ $newOps[] = $newOp;
138124 }
 125+ return $newOps;
 126+ }
139127
140 - // Finish each operation...
141 - foreach ( $performOps as $index => $fileOp ) {
142 - if ( isset( $failedOps[$index] ) ) {
143 - continue; // nothing to do
144 - }
145 - $subStatus = $fileOp->finish();
146 - if ( $subStatus->isOK() ) {
147 - ++$status->successCount;
148 - $status->success[$index] = true;
149 - } else {
150 - ++$status->failCount;
151 - $status->success[$index] = false;
152 - }
153 - $status->merge( $subStatus );
 128+ /**
 129+ * Replace the backend part of storage paths with this backend's name
 130+ *
 131+ * @param &$paths Array
 132+ * @return void
 133+ */
 134+ protected function unsubstPaths( array &$paths ) {
 135+ foreach ( $paths as &$path ) {
 136+ $path = preg_replace( '!^mwstore://([^/]+)!', "mwstore://{$this->name}", $path );
154137 }
155 -
156 - // Make sure status is OK, despite any finish() fatals
157 - $status->setResult( true, $status->value );
158 -
159 - return $status;
160138 }
161139
162140 function prepare( array $params ) {
163141 $status = Status::newGood();
164142 foreach ( $this->backends as $backend ) {
165 - $status->merge( $backend->prepare( $params ) );
 143+ $realParams = $this->substOpPaths( $params, $backend );
 144+ $status->merge( $backend->prepare( $realParams ) );
166145 }
167146 return $status;
168147 }
——@@ -169,7 +148,8 @@
170149 function secure( array $params ) {
171150 $status = Status::newGood();
172151 foreach ( $this->backends as $backend ) {
173 - $status->merge( $backend->secure( $params ) );
 152+ $realParams = $this->substOpPaths( $params, $backend );
 153+ $status->merge( $backend->secure( $realParams ) );
174154 }
175155 return $status;
176156 }
——@@ -177,7 +157,8 @@
178158 function clean( array $params ) {
179159 $status = Status::newGood();
180160 foreach ( $this->backends as $backend ) {
181 - $status->merge( $backend->clean( $params ) );
 161+ $realParams = $this->substOpPaths( $params, $backend );
 162+ $status->merge( $backend->clean( $realParams ) );
182163 }
183164 return $status;
184165 }
——@@ -185,7 +166,8 @@
186167 function fileExists( array $params ) {
187168 # Hit all backends in case of failed operations (out of sync)
188169 foreach ( $this->backends as $backend ) {
189 - if ( $backend->fileExists( $params ) ) {
 170+ $realParams = $this->substOpPaths( $params, $backend );
 171+ if ( $backend->fileExists( $realParams ) ) {
190172 return true;
191173 }
192174 }
——@@ -194,13 +176,15 @@
195177
196178 function getFileTimestamp( array $params ) {
197179 // Skip non-master for consistent timestamps
198 - return $this->backends[$this->masterIndex]->getFileTimestamp( $params );
 180+ $realParams = $this->substOpPaths( $params, $backend );
 181+ return $this->backends[$this->masterIndex]->getFileTimestamp( $realParams );
199182 }
200183
201184 function getSha1Base36(array $params) {
202185 # Hit all backends in case of failed operations (out of sync)
203186 foreach ( $this->backends as $backend ) {
204 - $hash = $backend->getSha1Base36( $params );
 187+ $realParams = $this->substOpPaths( $params, $backend );
 188+ $hash = $backend->getSha1Base36( $realParams );
205189 if ( $hash !== false ) {
206190 return $hash;
207191 }
——@@ -211,7 +195,8 @@
212196 function getFileProps( array $params ) {
213197 # Hit all backends in case of failed operations (out of sync)
214198 foreach ( $this->backends as $backend ) {
215 - $props = $backend->getFileProps( $params );
 199+ $realParams = $this->substOpPaths( $params, $backend );
 200+ $props = $backend->getFileProps( $realParams );
216201 if ( $props !== null ) {
217202 return $props;
218203 }
——@@ -222,7 +207,8 @@
223208 function streamFile( array $params ) {
224209 $status = Status::newGood();
225210 foreach ( $this->backends as $backend ) {
226 - $subStatus = $backend->streamFile( $params );
 211+ $realParams = $this->substOpPaths( $params, $backend );
 212+ $subStatus = $backend->streamFile( $realParams );
227213 $status->merge( $subStatus );
228214 if ( $subStatus->isOK() ) {
229215 // Pass isOK() despite fatals from other backends
——@@ -242,7 +228,8 @@
243229 function getLocalReference( array $params ) {
244230 # Hit all backends in case of failed operations (out of sync)
245231 foreach ( $this->backends as $backend ) {
246 - $fsFile = $backend->getLocalReference( $params );
 232+ $realParams = $this->substOpPaths( $params, $backend );
 233+ $fsFile = $backend->getLocalReference( $realParams );
247234 if ( $fsFile ) {
248235 return $fsFile;
249236 }
——@@ -253,7 +240,8 @@
254241 function getLocalCopy( array $params ) {
255242 # Hit all backends in case of failed operations (out of sync)
256243 foreach ( $this->backends as $backend ) {
257 - $tmpFile = $backend->getLocalCopy( $params );
 244+ $realParams = $this->substOpPaths( $params, $backend );
 245+ $tmpFile = $backend->getLocalCopy( $realParams );
258246 if ( $tmpFile ) {
259247 return $tmpFile;
260248 }
——@@ -264,7 +252,8 @@
265253 function getFileList( array $params ) {
266254 foreach ( $this->backends as $index => $backend ) {
267255 # Get results from the first backend
268 - return $backend->getFileList( $params );
 256+ $realParams = $this->substOpPaths( $params, $backend );
 257+ return $backend->getFileList( $realParams );
269258 }
270259 return array(); // sanity
271260 }
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileOp.php
——@@ -64,6 +64,14 @@
6565 }
6666
6767 /**
 68+ * Check if this operation failed precheck() or attempt()
 69+ * @return type
 70+ */
 71+ final public function failed() {
 72+ return $this->failed;
 73+ }
 74+
 75+ /**
6876 * Get a new empty predicates array for precheck()
6977 *
7078 * @return Array
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
——@@ -150,6 +150,77 @@
151151 }
152152
153153 /**
 154+ * Attempt a series of file operations.
 155+ * Callers are responsible for handling file locking.
 156+ *
 157+ * @param $performOps Array List of FileOp operations
 158+ * @return Status
 159+ */
 160+ protected function attemptOperations( array $performOps ) {
 161+ $status = Status::newGood();
 162+
 163+ $predicates = FileOp::newPredicates(); // account for previous op in prechecks
 164+ // Do pre-checks for each operation; abort on failure...
 165+ foreach ( $performOps as $index => $fileOp ) {
 166+ $status->merge( $fileOp->precheck( $predicates ) );
 167+ if ( !$status->isOK() ) { // operation failed?
 168+ if ( $fileOp->getParam( 'ignoreErrors' ) ) {
 169+ ++$status->failCount;
 170+ $status->success[$index] = false;
 171+ } else {
 172+ return $status;
 173+ }
 174+ }
 175+ }
 176+
 177+ // Attempt each operation; abort on failure...
 178+ foreach ( $performOps as $index => $fileOp ) {
 179+ if ( $fileOp->failed() ) {
 180+ continue; // nothing to do
 181+ }
 182+ $status->merge( $fileOp->attempt() );
 183+ if ( !$status->isOK() ) { // operation failed?
 184+ if ( $fileOp->getParam( 'ignoreErrors' ) ) {
 185+ ++$status->failCount;
 186+ $status->success[$index] = false;
 187+ } else {
 188+ // Revert everything done so far and abort.
 189+ // Do this by reverting each previous operation in reverse order.
 190+ $pos = $index - 1; // last one failed; no need to revert()
 191+ while ( $pos >= 0 ) {
 192+ if ( !$performOps[$pos]->failed() ) {
 193+ $status->merge( $performOps[$pos]->revert() );
 194+ }
 195+ $pos--;
 196+ }
 197+ return $status;
 198+ }
 199+ }
 200+ }
 201+
 202+ // Finish each operation...
 203+ foreach ( $performOps as $index => $fileOp ) {
 204+ if ( $fileOp->failed() ) {
 205+ continue; // nothing to do
 206+ }
 207+ $subStatus = $fileOp->finish();
 208+ if ( $subStatus->isOK() ) {
 209+ ++$status->successCount;
 210+ $status->success[$index] = true;
 211+ } else {
 212+ ++$status->failCount;
 213+ $status->success[$index] = false;
 214+ }
 215+ $status->merge( $subStatus );
 216+ }
 217+
 218+ // Make sure status is OK, despite any finish() fatals
 219+ $status->setResult( true, $status->value );
 220+
 221+ return $status;
 222+ }
 223+
 224+ /**
154225 * Prepare a storage path for usage. This will create containers
155226 * that don't yet exist or, on FS backends, create parent directories.
156227 *
——@@ -461,7 +532,7 @@
462533 if ( !$fsFile ) {
463534 return false;
464535 } else {
465 - return $fsFile->sha1Base36();
 536+ return $fsFile->getSha1Base36();
466537 }
467538 }
468539
——@@ -549,84 +620,28 @@
550621 }
551622
552623 final public function doOperations( array $ops ) {
553 - $status = Status::newGood( array() );
 624+ $status = Status::newGood();
554625
555626 // Build up a list of FileOps...
556627 $performOps = $this->getOperations( $ops );
557628
558629 // Build up a list of files to lock...
559 - $filesToLock = array();
 630+ $filesLockEx = $filesLockSh = array();
560631 foreach ( $performOps as $index => $fileOp ) {
561 - $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsUsed() );
 632+ $filesLockSh = array_merge( $filesLockSh, $fileOp->storagePathsRead() );
 633+ $filesLockEx = array_merge( $filesLockEx, $fileOp->storagePathsChanged() );
562634 }
563635
564636 // Try to lock those files for the scope of this function...
565 - $scopedLock = $this->getScopedFileLocks( $filesToLock, LockManager::LOCK_EX, $status );
 637+ $scopeLockS = $this->getScopedFileLocks( $filesLockSh, LockManager::LOCK_UW, $status );
 638+ $scopeLockE = $this->getScopedFileLocks( $filesLockEx, LockManager::LOCK_EX, $status );
566639 if ( !$status->isOK() ) {
567640 return $status; // abort
568641 }
569642
570 - $failedOps = array(); // failed ops with 'ignoreErrors'
571 - $predicates = FileOp::newPredicates(); // account for previous op in prechecks
572 - // Do pre-checks for each operation; abort on failure...
573 - foreach ( $performOps as $index => $fileOp ) {
574 - $status->merge( $fileOp->precheck( $predicates ) );
575 - if ( !$status->isOK() ) { // operation failed?
576 - if ( $fileOp->getParam( 'ignoreErrors' ) ) {
577 - $failedOps[$index] = 1; // remember not to call attempt()/finish()
578 - ++$status->failCount;
579 - $status->success[$index] = false;
580 - } else {
581 - return $status;
582 - }
583 - }
584 - }
 643+ // Actually attempt the operation batch...
 644+ $status->merge( $this->attemptOperations( $performOps ) );
585645
586 - // Attempt each operation; abort on failure...
587 - foreach ( $performOps as $index => $fileOp ) {
588 - if ( isset( $failedOps[$index] ) ) {
589 - continue; // nothing to do
590 - }
591 - $status->merge( $fileOp->attempt() );
592 - if ( !$status->isOK() ) { // operation failed?
593 - if ( $fileOp->getParam( 'ignoreErrors' ) ) {
594 - $failedOps[$index] = 1; // remember not to call finish()
595 - ++$status->failCount;
596 - $status->success[$index] = false;
597 - } else {
598 - // Revert everything done so far and abort.
599 - // Do this by reverting each previous operation in reverse order.
600 - $pos = $index - 1; // last one failed; no need to revert()
601 - while ( $pos >= 0 ) {
602 - if ( !isset( $failedOps[$pos] ) ) {
603 - $status->merge( $performOps[$pos]->revert() );
604 - }
605 - $pos--;
606 - }
607 - return $status;
608 - }
609 - }
610 - }
611 -
612 - // Finish each operation...
613 - foreach ( $performOps as $index => $fileOp ) {
614 - if ( isset( $failedOps[$index] ) ) {
615 - continue; // nothing to do
616 - }
617 - $subStatus = $fileOp->finish();
618 - if ( $subStatus->isOK() ) {
619 - ++$status->successCount;
620 - $status->success[$index] = true;
621 - } else {
622 - ++$status->failCount;
623 - $status->success[$index] = false;
624 - }
625 - $status->merge( $subStatus );
626 - }
627 -
628 - // Make sure status is OK, despite any finish() fatals
629 - $status->setResult( true, $status->value );
630 -
631646 return $status;
632647 }
633648
Index: branches/FileBackend/phase3/includes/filerepo/FileRepo.php
——@@ -1200,7 +1200,7 @@
12011201 if ( !$tmpFile ) {
12021202 return false;
12031203 }
1204 - return $tmpFile->sha1Base36();
 1204+ return $tmpFile->getSha1Base36();
12051205 }
12061206
12071207 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r107304Fixed undefined var from r106280aaron23:17, 25 December 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   04:01, 16 December 2011

Actually, FileBackendMultiWrite constructs the sub-backends itself to hide them from registration, so we don't care about that. It's still useful to give them their own names though, as the constructor allows.

#Comment by Reedy (talk | contribs)   22:30, 25 December 2011

Marking fixme as this is now an issue in core.

Line 208 of FileBackendMultiWrite.php uses a undefined $backend variable

Status & tagging log