r103790 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r103789‎ | r103790 | r103791 >
Date:22:03, 20 November 2011
Author:aaron
Status:deferred
Tags:
Comment:
* Cleanups to DBFileLockManager; improved $config layout
* Moved locking to doOperations() to reduce queries for DB-based locking (better batching)
* Renamed $transaction to $fileOp in doOperations()
* Fixed bug in attempt() wrt setting failedAttempt field
* Added missing storagePathsToLock() to FileMoveOp
* Better use of Status::merge() function in various places
Modified paths:
  • /branches/FileBackend/phase3/includes/filerepo/ForeignDBViaLBRepo.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/FileLockManager.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
——@@ -1,7 +1,5 @@
22 <?php
33 /**
4 - * Class for a "backend" consisting of a orioritized list of backend
5 - *
64 * @file
75 * @ingroup FileRepo
86 */
——@@ -41,35 +39,56 @@
4240
4341 final public function doOperations( array $ops ) {
4442 $status = Status::newGood();
 43+
4544 // Build up a list of FileOps. The list will have all the ops
4645 // for one backend, then all the ops for the next, and so on.
 46+ // Also build up a list of files to lock...
4747 $performOps = array();
48 - foreach ( $this->fileBackends as $backend ) {
 48+ $filesToLock = array();
 49+ foreach ( $this->fileBackends as $index => $backend ) {
4950 $performOps = array_merge( $performOps, $backend->getOperations( $ops ) );
 51+ // Set $filesToLock from the first backend so we don't try to set all
 52+ // locks two or three times (depending on the number of backends).
 53+ if ( $index == 0 ) {
 54+ foreach ( $performOps as $index => $fileOp ) {
 55+ $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsToLock() );
 56+ }
 57+ $filesToLock = array_unique( $filesToLock ); // avoid warnings
 58+ }
5059 }
 60+
 61+ // Try to lock those files...
 62+ $status->merge( $this->lockFiles( $filesToLock ) );
 63+ if ( !$status->isOK() ) {
 64+ return $status; // abort
 65+ }
 66+
5167 // Attempt each operation; abort on failure...
52 - foreach ( $performOps as $index => $transaction ) {
53 - $subStatus = $transaction->attempt();
54 - $status->merge( $subStatus );
55 - if ( !$subStatus->isOK() ) { // operation failed?
 68+ foreach ( $performOps as $index => $fileOp ) {
 69+ $status->merge( $fileOp->attempt() );
 70+ if ( !$status->isOK() ) { // operation failed?
5671 // Revert everything done so far and abort.
5772 // Do this by reverting each previous operation in reverse order.
5873 $pos = $index - 1; // last one failed; no need to revert()
5974 while ( $pos >= 0 ) {
60 - $subStatus = $performOps[$pos]->revert();
61 - $status->merge( $subStatus );
 75+ $status->merge( $performOps[$pos]->revert() );
6276 $pos--;
6377 }
6478 return $status;
6579 }
6680 }
 81+
6782 // Finish each operation...
68 - foreach ( $performOps as $index => $transaction ) {
69 - $subStatus = $transaction->finish();
70 - $status->merge( $subStatus );
 83+ foreach ( $performOps as $index => $fileOp ) {
 84+ $status->merge( $fileOp->finish() );
7185 }
72 - // Make sure status is OK, despite any finish() fatals
 86+
 87+ // Unlock all the files
 88+ $status->merge( $this->unlockFiles( $filesToLock ) );
 89+
 90+ // Make sure status is OK, despite any finish() or unlockFiles() fatals
7391 $status->setResult( true );
 92+
7493 return $status;
7594 }
7695
——@@ -145,13 +164,11 @@
146165 function lockFiles( array $paths ) {
147166 $status = Status::newGood();
148167 foreach ( $this->backends as $index => $backend ) {
149 - $subStatus = $backend->lockFiles( $paths );
150 - $status->merge( $subStatus );
151 - if ( !$subStatus->isOk() ) {
 168+ $status->merge( $backend->lockFiles( $paths ) );
 169+ if ( !$status->isOk() ) {
152170 // Lock failed: release the locks done so far each backend
153171 for ( $i=0; $i < $index; $i++ ) { // don't do backend $index since it failed
154 - $subStatus = $backend->unlockFiles( $paths );
155 - $status->merge( $subStatus );
 172+ $status->merge( $backend->unlockFiles( $paths ) );
156173 }
157174 return $status;
158175 }
——@@ -162,8 +179,7 @@
163180 function unlockFiles( array $paths ) {
164181 $status = Status::newGood();
165182 foreach ( $this->backends as $backend ) {
166 - $subStatus = $backend->unlockFile( $paths );
167 - $status->merge( $subStatus );
 183+ $status->merge( $backend->unlockFile( $paths ) );
168184 }
169185 return $status;
170186 }
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileLockManager.php
——@@ -97,8 +97,7 @@
9898 $lockedKeys[] = $key;
9999 } else {
100100 // Abort and unlock everything
101 - $subStatus = $this->doUnlock( $lockedKeys );
102 - $status->merge( $subStatus );
 101+ $status->merge( $this->doUnlock( $lockedKeys ) );
103102 return $status;
104103 }
105104 }
——@@ -110,8 +109,7 @@
111110 $status = Status::newGood();
112111
113112 foreach ( $keys as $key ) {
114 - $subStatus = $this->doSingleUnlock( $key );
115 - $status->merge( $subStatus );
 113+ $status->merge( $this->doSingleUnlock( $key ) );
116114 }
117115
118116 return $status;
——@@ -181,21 +179,19 @@
182180 * One or several lock database servers are set up having a `file_locking`
183181 * table with one field, fl_key, the PRIMARY KEY. The table engine should
184182 * have row-level locking. For performance, deadlock detection should be
185 - * disabled and a low lock-wait timeout should be put in the server config.
 183+ * disabled and a low lock-wait timeout should be set via server config.
186184 *
187185 * All lock requests for an item (identified by an abstract key string) will
188 - * map to one bucket. Each bucket maps to a single server, and each server can
189 - * have several or no fallback servers. Fallback servers recieve the same lock
190 - * statements as the servers they are set as fallbacks for. This propagation is
191 - * only best-effort; lock requests will not be blocked just because a fallback
192 - * server cannot be contacted to recieve a copy of the lock request.
 186+ * map to one bucket. Each bucket maps to a single server, though each server
 187+ * can have several fallback servers.
 188+ *
 189+ * Fallback servers recieve the same lock statements as the servers they standby for.
 190+ * This propagation is only best-effort; lock requests will not be blocked just
 191+ * because a fallback server cannot recieve a copy of the lock request.
193192 */
194193 class DBFileLockManager extends FileLockManager {
195194 /** @var Array Map of bucket indexes to server names */
196 - protected $serverMap = array(); // (index => server name)
197 - /** @var Array Map of servers to fallback server names */
198 - protected $serverFallbackMap = array(); // (server => (server1,server2,...))
199 -
 195+ protected $serverMap = array(); // (index => (server1,server2,...))
200196 /** @var Array List of active lock key names */
201197 protected $locksHeld = array(); // (key => 1)
202198 /** $var Array Map Lock-active database connections (name => Database) */
——@@ -203,54 +199,64 @@
204200
205201 /**
206202 * Construct a new instance from configuration.
207 - * The 'serverMap' param of $config has is an array of consecutive
208 - * integer keys, starting from 0, with server name strings as values.
209 - * It should have no more than 16 items in the array.
210 - *
211 - * The `file_locking` table should have row-level locking (e.g. innoDB).
212 - *
213 - * @param array $config
 203+ * $config paramaters include:
 204+ * 'serverMap' : Array of no more than 16 consecutive integer keys,
 205+ * starting from 0, with a list of servers as values.
 206+ * The first server in each list is the main server and
 207+ * the others are fallback servers.
 208+ *
 209+ * @param Array $config
214210 */
215211 function __construct( array $config ) {
216 - $this->serverMap = $config['serverMap'];
217 - $this->serverFallbackMap = $config['serverFallbackMap'];
 212+ $this->serverMap = (array)$config['serverMap'];
 213+ // Sanitize serverMap against bad config to prevent PHP errors
 214+ for ( $i=0; $i < count( $this->serverMap ); $i++ ) {
 215+ if (
 216+ !isset( $this->serverMap[$i] ) || // not consecutive
 217+ !is_array( $this->serverMap[$i] ) || // bad type
 218+ !count( $this->serverMap[$i] ) // empty list
 219+ ) {
 220+ $this->serverMap[$i] = null; // see getBucketFromKey()
 221+ wfWarn( "No key for bucket $i in serverMap or server list is empty." );
 222+ }
 223+ }
218224 }
219225
220226 function doLock( array $keys ) {
221227 $status = Status::newGood();
222228
223229 $keysToLock = array();
224 - // Get locks that need to be acquired and which server they map to...
 230+ // Get locks that need to be acquired (buckets => locks)...
225231 foreach ( $keys as $key ) {
226232 if ( isset( $this->locksHeld[$key] ) ) {
227233 $status->warning( 'File already locked.' );
228234 } else {
229 - $server = $this->getDBServerFromKey( $key );
230 - if ( $server === null ) { // config error?
231 - $status->fatal( "Lock server for $key is not set." );
 235+ $bucket = $this->getBucketFromKey( $key );
 236+ if ( $bucket === null ) { // config error?
 237+ $status->fatal( "Lock servers for key $key is not set." );
232238 return $status;
233239 }
234 - $keysToLock[$server][] = $key;
 240+ $keysToLock[$bucket][] = $key;
235241 }
236242 }
237243
238244 $lockedKeys = array(); // files locked in this attempt
239245 // Attempt to acquire these locks...
240 - foreach ( $keysToLock as $server => $keys ) {
 246+ foreach ( $keysToLock as $bucket => $keys ) {
 247+ $server = $this->serverMap[$bucket][0]; // primary lock server
 248+ $propagateToFallbacks = true; // give lock statements to fallback servers
241249 // Acquire the locks for this server. Three main cases can happen:
242250 // (a) Server is up; common case
243251 // (b) Server is down but a fallback is up
244252 // (c) Server is down and no fallbacks are up (or none defined)
245 - $propagateToFallbacks = true;
246253 try {
247254 $this->lockingSelect( $server, $keys ); // SELECT FOR UPDATE
248255 } catch ( DBError $e ) {
249256 // Can we manage to lock on any of the fallback servers?
250 - if ( !$this->lockingSelectFallbacks( $server, $keys ) ) {
251 - // Abort and unlock everything
 257+ if ( !$this->lockingSelectFallbacks( $bucket, $keys ) ) {
 258+ // Abort and unlock everything we just locked
252259 $status->fatal( "Could not contact the lock server." );
253 - $subStatus = $this->doUnlock( $lockedKeys );
254 - $status->merge( $subStatus );
 260+ $status->merge( $this->doUnlock( $lockedKeys ) );
255261 return $status;
256262 } else { // recovered using fallbacks
257263 $propagateToFallbacks = false; // done already
——@@ -258,7 +264,7 @@
259265 }
260266 // Propagate any locks to the fallback servers (best effort)
261267 if ( $propagateToFallbacks ) {
262 - $this->lockingSelectFallbacks( $server, $keys );
 268+ $this->lockingSelectFallbacks( $bucket, $keys );
263269 }
264270 // Record locks as active
265271 foreach ( $keys as $key ) {
——@@ -290,7 +296,7 @@
291297 }
292298
293299 /**
294 - * Get a database connection for $server and lock the rows for $keys
 300+ * Get a DB connection to a lock server and acquire locks on $keys.
295301 *
296302 * @param $server string
297303 * @param $keys Array
——@@ -311,24 +317,23 @@
312318 }
313319
314320 /**
315 - * Propagate any locks to the fallback servers for $server.
 321+ * Propagate any locks to the fallback servers for a bucket.
316322 * This should avoid throwing any exceptions.
317323 *
318 - * @param $server string
 324+ * @param $bucket integer
319325 * @param $keys Array
320326 * @return bool Locks made on at least one fallback server
321327 */
322 - protected function lockingSelectFallbacks( $server, array $keys ) {
 328+ protected function lockingSelectFallbacks( $bucket, array $keys ) {
323329 $locksMade = false;
324 - if ( isset( $this->serverFallbackMap[$server] ) ) {
325 - // Propagate the $server locks to each fallback for $server...
326 - foreach ( $this->serverFallbackMap[$server] as $fallbackServer ) {
327 - try {
328 - $this->doLockingSelect( $fallbackServer, $keys ); // SELECT FOR UPDATE
329 - $locksMade = true;
330 - } catch ( DBError $e ) {
331 - // oh well; best effort
332 - }
 330+ $count = count( $this->serverMap[$bucket] );
 331+ for ( $i=1; $i < $count; $i++ ) { // start at 1 to only include fallbacks
 332+ $server = $this->serverMap[$bucket][$i];
 333+ try {
 334+ $this->doLockingSelect( $server, $keys ); // SELECT FOR UPDATE
 335+ $locksMade = true; // success for this fallback
 336+ } catch ( DBError $e ) {
 337+ // oh well; best effort
333338 }
334339 }
335340 return $locksMade;
——@@ -341,14 +346,14 @@
342347 * @return void
343348 */
344349 protected function commitLockTransactions() {
345 - try {
346 - foreach ( $this->activeConns as $server => $db ) {
 350+ foreach ( $this->activeConns as $server => $db ) {
 351+ try {
347352 $db->commit(); // finish transaction
348 - unset( $this->activeConns[$server] );
 353+ } catch ( DBError $e ) {
 354+ // oh well
349355 }
350 - } catch ( DBError $e ) {
351 - // oh well
352356 }
 357+ $this->activeConns = array();
353358 }
354359
355360 /**
——@@ -356,29 +361,17 @@
357362 * This should avoid throwing any exceptions.
358363 *
359364 * @param $key string
360 - * @return int
 365+ * @return integer
361366 */
362367 protected function getBucketFromKey( $key ) {
363368 $hash = str_pad( md5( $key ), 32, '0', STR_PAD_LEFT ); // 32 char hash
364369 $prefix = substr( $hash, 0, 2 ); // first 2 hex chars (8 bits)
365 - return ( intval( base_convert( $prefix, 16, 10 ) ) % count( $this->serverMap ) );
366 - }
367 -
368 - /**
369 - * Get the server name for lock key.
370 - * This should avoid throwing any exceptions.
371 - *
372 - * @param $key string
373 - * @return string|null
374 - */
375 - protected function getDBServerFromKey( $key ) {
376 - $bucket = $this->getBucketFromKey( $key );
377 - if ( isset( $this->serverMap[$bucket] ) ) {
378 - return $this->serverMap[$bucket];
379 - } else {
380 - wfWarn( "No key for bucket $bucket in serverMap." );
381 - return null; // disabled? bad config?
 370+ $bucket = intval( base_convert( $prefix, 16, 10 ) ) % count( $this->serverMap );
 371+ // Sanity check that at least one server is handling this bucket
 372+ if ( !isset( $this->serverMap[$bucket] ) ) {
 373+ return null; // bad config?
382374 }
 375+ return $bucket;
383376 }
384377 }
385378
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
——@@ -258,7 +258,7 @@
259259 // Get params for this operation
260260 $params = $operation;
261261 unset( $params['operation'] ); // don't need this
262 - // Add operation on to the list of things to do
 262+ // Append the FileOp class
263263 $performOps[] = new $class( $params );
264264 } else {
265265 throw new MWException( "Operation `$opName` is not supported." );
——@@ -270,31 +270,49 @@
271271
272272 final public function doOperations( array $ops ) {
273273 $status = Status::newGood();
 274+
274275 // Build up a list of FileOps...
275276 $performOps = $this->getOperations( $ops );
 277+
 278+ // Build up a list of files to lock...
 279+ $filesToLock = array();
 280+ foreach ( $performOps as $index => $fileOp ) {
 281+ $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsToLock() );
 282+ }
 283+ $filesToLock = array_unique( $filesToLock ); // avoid warnings
 284+
 285+ // Try to lock those files...
 286+ $status->merge( $this->lockFiles( $filesToLock ) );
 287+ if ( !$status->isOK() ) {
 288+ return $status; // abort
 289+ }
 290+
276291 // Attempt each operation; abort on failure...
277 - foreach ( $performOps as $index => $transaction ) {
278 - $subStatus = $transaction->attempt();
279 - $status->merge( $subStatus );
280 - if ( !$subStatus->isOK() ) { // operation failed?
 292+ foreach ( $performOps as $index => $fileOp ) {
 293+ $status->merge( $fileOp->attempt() );
 294+ if ( !$status->isOK() ) { // operation failed?
281295 // Revert everything done so far and abort.
282296 // Do this by reverting each previous operation in reverse order.
283297 $pos = $index - 1; // last one failed; no need to revert()
284298 while ( $pos >= 0 ) {
285 - $subStatus = $performOps[$pos]->revert();
286 - $status->merge( $subStatus );
 299+ $status->merge( $performOps[$pos]->revert() );
287300 $pos--;
288301 }
289302 return $status;
290303 }
291304 }
 305+
292306 // Finish each operation...
293 - foreach ( $performOps as $index => $transaction ) {
294 - $subStatus = $transaction->finish();
295 - $status->merge( $subStatus );
 307+ foreach ( $performOps as $index => $fileOp ) {
 308+ $status->merge( $fileOp->finish() );
296309 }
297 - // Make sure status is OK, despite any finish() fatals
 310+
 311+ // Unlock all the files
 312+ $status->merge( $this->unlockFiles( $filesToLock ) );
 313+
 314+ // Make sure status is OK, despite any finish() or unlockFiles() fatals
298315 $status->setResult( true );
 316+
299317 return $status;
300318 }
301319
——@@ -354,10 +372,8 @@
355373 throw new MWException( "Cannot attempt operation called twice." );
356374 }
357375 $this->state = self::STATE_ATTEMPTED;
358 - $status = $this->setLocks();
359 - if ( $status->isOK() ) {
360 - $status = $this->doAttempt();
361 - } else {
 376+ $status = $this->doAttempt();
 377+ if ( !$status->isOK() ) {
362378 $this->failedAttempt = true;
363379 }
364380 return $status;
——@@ -373,12 +389,11 @@
374390 throw new MWException( "Cannot rollback an unstarted or finished operation." );
375391 }
376392 $this->state = self::STATE_DONE;
377 - if ( !$this->failedAttempt ) {
 393+ if ( $this->failedAttempt ) {
 394+ $status = Status::newGood(); // nothing to revert
 395+ } else {
378396 $status = $this->doRevert();
379 - } else {
380 - $status = Status::newGood(); // nothing to revert
381397 }
382 - $this->unsetLocks();
383398 return $status;
384399 }
385400
——@@ -392,39 +407,20 @@
393408 throw new MWException( "Cannot cleanup an unstarted or finished operation." );
394409 }
395410 $this->state = self::STATE_DONE;
396 - if ( !$this->failedAttempt ) {
 411+ if ( $this->failedAttempt ) {
 412+ $status = Status::newGood(); // nothing to revert
 413+ } else {
397414 $status = $this->doFinish();
398 - } else {
399 - $status = Status::newGood(); // nothing to revert
400415 }
401 - $this->unsetLocks();
402416 return $status;
403417 }
404418
405419 /**
406 - * Try to lock any files before changing them
407 - *
408 - * @return Status
409 - */
410 - private function setLocks() {
411 - return $this->backend->lockFiles( $this->storagePathsToLock() );
412 - }
413 -
414 - /**
415 - * Try to unlock any files that this locked
416 - *
417 - * @return Status
418 - */
419 - private function unsetLocks() {
420 - return $this->backend->unlockFiles( $this->storagePathsToLock() );
421 - }
422 -
423 - /**
424420 * Get a list of storage paths to lock for this operation
425421 *
426422 * @return Array
427423 */
428 - protected function storagePathsToLock() {
 424+ public function storagePathsToLock() {
429425 return array();
430426 }
431427
——@@ -632,6 +628,10 @@
633629 }
634630 return $status;
635631 }
 632+
 633+ function storagePathsToLock() {
 634+ return array( $this->params['source'], $this->params['dest'] );
 635+ }
636636 }
637637
638638 /**
Index: branches/FileBackend/phase3/includes/filerepo/ForeignDBViaLBRepo.php
——@@ -30,6 +30,7 @@
3131 function getSlaveDB() {
3232 return wfGetDB( DB_SLAVE, array(), $this->wiki );
3333 }
 34+
3435 function hasSharedCache() {
3536 return $this->hasSharedCache;
3637 }

Status & tagging log