| Index: trunk/extensions/SwiftMedia/TODO |
| — | — | @@ -1,5 +1,6 @@ |
| 2 | 2 | Pending: |
| 3 | 3 | |
| | 4 | +This group all relate to the 404 handler: |
| 4 | 5 | 6) There's no 404 handler to generate missing thumbnails. |
| 5 | 6 | 7) There's no support for remote thumbnailing. |
| 6 | 7 | 7.1) The SpecialUploadStash, when it calls the remote scaler, is actually just relying on the 404 handler on upload.wm.org. |
| — | — | @@ -9,16 +10,11 @@ |
| 10 | 11 | front-ends forwards the request to a thumb.php running on the scaler cluster. Thumb.php takes care of creating the thumbnail. |
| 11 | 12 | 6&7) Basically, the code which currently fetches 404 thumbs from upload.wikimedia.org needs to be changed slightly so that it inserts |
| 12 | 13 | thumb.php with the appropriate parameters and fetches from the scaler cluster. |
| 13 | | -8) Test cases (but of course that could be done until the cows come home). |
| 14 | | -9) Read through the code and look for anything which is insane. |
| 15 | | -10) Remove directory from $wgLocalFileRepo, to make sure that there's no references to it. Ditto for wgDeletedDirectory and deletedDir. |
| 16 | | -11) Determine what to do about the one remaining core change needed for Swift. |
| | 14 | +19) TS: "I suggest you test transform errors throughout your whole system. With our current NFS system, transform errors work by having thumb.php return an HTTP 500 response with an informative HTML error message. The error message is passed through to Squid by the 404 handler, and Squid won't cache it. The user will see a broken image icon in their browser, and choosing "view image" from the context menu of the image will show them the error message from thumb.php." |
| | 15 | + |
| 17 | 16 | 12) Why is anybody calling resolveVirtualUrl()? It's defined in the Repo, but getPath() is defined against a file. |
| 18 | 17 | Why is UploadStashFile() being called with a virtual URL? Once the file has been stashed() it has an object name. The container name is implicit. |
| 19 | 18 | Should UploadStashFile *always* (in our case) be called with a virtual URL? |
| 20 | | -19) TS: "I suggest you test transform errors throughout your whole system. With our current NFS system, transform errors work by having thumb.php return an HTTP 500 response with an informative HTML error message. The error message is passed through to Squid by the 404 handler, and Squid won't cache it. The user will see a broken image icon in their browser, and choosing "view image" from the context menu of the image will show them the error message from thumb.php." |
| 21 | | -20) TS: "It's not appropriate to allow CloudFiles exceptions to be propagated back to the callers of File/FileRepo methods such as transform(). Doing this will cause the page to not be displayed at all, with an exceptionally ugly error message in its place. FileRepo has a system for returning user-friendly error messages from pretty much anywhere. For example, SwiftRepo::storeBatch() has a Status object to hold error messages, but your code just sets it to a "good" result, it never sets any errors in it." |
| 22 | | -"You can throw an MWException in response to configuration errors, or assertion-like unexpected errors, but it's not appropriate to be throwing exceptions in response to network errors or errors generated by the Swift server. |
| 23 | 19 | 21) TS: "// Check overwriting |
| 24 | 20 | if (0) { #FIXME |
| 25 | 21 | |
| — | — | @@ -26,15 +22,21 @@ |
| 27 | 23 | 22) TS: "I think this [MD65] validation feature in FSRepo is just a hack for NFS. Okay to remove it." |
| 28 | 24 | 23) TS: "When you get around to implementing SwiftRepo::append(), it will need some sort of concurrency control to avoid having chunks overwrite each other. " |
| 29 | 25 | 24) TS: "SwiftRepo::swiftcopy() should return a Status object instead of throwing exceptions." |
| 30 | | -25) TS: In the short term, just copy ForeignDBViaLBRepo to ForeignDBViaSMRepo and change the class parent. |
| 31 | 26 | |
| | 27 | +Partially resolved: |
| 32 | 28 | |
| | 29 | +20) TS: "It's not appropriate to allow CloudFiles exceptions to be propagated back to the callers of File/FileRepo methods such as transform(). Doing this will cause the page to not be displayed at all, with an exceptionally ugly error message in its place. FileRepo has a system for returning user-friendly error messages from pretty much anywhere. For example, SwiftRepo::storeBatch() has a Status object to hold error messages, but your code just sets it to a "good" result, it never sets any errors in it." |
| | 30 | +"You can throw an MWException in response to configuration errors, or assertion-like unexpected errors, but it's not appropriate to be throwing exceptions in response to network errors or errors generated by the Swift server. |
| | 31 | + |
| 33 | 32 | Resolved: |
| 34 | 33 | |
| 35 | 34 | 5) The Upload seems to take more time than I expect, but that could be a function of generating the six thumbnails. |
| 36 | 35 | It *is* a function of generating the seven (we generate 800x600 twice) thumbnails. Each one takes 1/2 second. |
| | 36 | +8) Test cases (but of course that could be done until the cows come home). |
| | 37 | +9) Read through the code and look for anything which is insane. |
| 37 | 38 | 10) Remove directory from $wgLocalFileRepo, to make sure that there's no references to it. Ditto for wgDeletedDirectory and deletedDir. |
| 38 | 39 | wgDeletedDirectory and deletedDir can be removed. |
| | 40 | +11) Determine what to do about the one remaining core change needed for Swift. |
| 39 | 41 | 12) Implement repo->freeTemp() - needed by several extensions and UploadFromStash. |
| 40 | 42 | 13) Do we need $wgLocalRepo->ThumbUrl to be configurable given that the Python middleware presumes it? |
| 41 | 43 | We currently have no need for it to be configurable in Swift. I'll just hard-code it to .../thumb with a note saying |
| — | — | @@ -46,6 +48,7 @@ |
| 47 | 49 | 18) TS: "You need to handle errors from MediaHandler::doTransform() correctly. " |
| 48 | 50 | 20) TS: "SwiftMedia::migrateThumbFile() should just do nothing," |
| 49 | 51 | 21) TS: "[$wgExcludeFromThumbnailPurge] looks pretty trivial to implement to me." (RN: It was!) |
| | 52 | +25) TS: In the short term, just copy ForeignDBViaLBRepo to ForeignDBViaSMRepo and change the class parent. |
| 50 | 53 | |
| 51 | 54 | |
| 52 | 55 | |
| Index: trunk/extensions/SwiftMedia/SwiftMedia.body.php |
| — | — | @@ -140,7 +140,7 @@ |
| 141 | 141 | * |
| 142 | 142 | * @return MediaTransformOutput | false |
| 143 | 143 | */ |
| 144 | | - private function maybeDoTransform( $thumbName, $thumbUrl, $params, $flags ) { |
| | 144 | + function maybeDoTransform( $thumbName, $thumbUrl, $params, $flags ) { |
| 145 | 145 | global $wgIgnoreImageErrors, $wgThumbnailEpoch, $wgTmpDirectory; |
| 146 | 146 | |
| 147 | 147 | // get a temporary place to put the original. |
| — | — | @@ -203,7 +203,7 @@ |
| 204 | 204 | /** |
| 205 | 205 | * We have nothing to do here. |
| 206 | 206 | */ |
| 207 | | - protected function migrateThumbFile( $thumbName ) { |
| | 207 | + function migrateThumbFile( $thumbName ) { |
| 208 | 208 | return; |
| 209 | 209 | } |
| 210 | 210 | /** |
| — | — | @@ -316,14 +316,14 @@ |
| 317 | 317 | * |
| 318 | 318 | * @return CF_Connection |
| 319 | 319 | */ |
| 320 | | - protected function connect() { |
| | 320 | + function connect() { |
| 321 | 321 | $auth = new CF_Authentication($this->swiftuser, $this->swiftkey, NULL, $this->authurl); |
| 322 | 322 | try { |
| 323 | 323 | $auth->authenticate(); |
| 324 | 324 | } catch (AuthenticationException $e) { |
| 325 | | - throw new MWException( 'We can't authenticate ourselves.' ); |
| 326 | | - } catch (InvalidResponseException $e) { |
| 327 | | - throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| | 325 | + throw new MWException( "We can't authenticate ourselves." ); |
| | 326 | + #} catch (InvalidResponseException $e) { |
| | 327 | + # throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| 328 | 328 | } |
| 329 | 329 | return new CF_Connection($auth); |
| 330 | 330 | } |
| — | — | @@ -334,13 +334,13 @@ |
| 335 | 335 | * |
| 336 | 336 | * @return CF_Container |
| 337 | 337 | */ |
| 338 | | - protected function get_container($conn, $cont) { |
| | 338 | + function get_container($conn, $cont) { |
| 339 | 339 | try { |
| 340 | 340 | return $conn->get_container($cont); |
| 341 | 341 | } catch (NoSuchContainerException $e) { |
| 342 | 342 | throw new MWException( "A container we thought existed, doesn't." ); |
| 343 | | - } catch (InvalidResponseException $e) { |
| 344 | | - throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| | 343 | + #} catch (InvalidResponseException $e) { |
| | 344 | + # throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| 345 | 345 | } |
| 346 | 346 | } |
| 347 | 347 | |
| — | — | @@ -351,7 +351,7 @@ |
| 352 | 352 | * |
| 353 | 353 | * @return CF_Container |
| 354 | 354 | */ |
| 355 | | - protected function write_swift_object( $srcPath, $dstc, $dstRel) { |
| | 355 | + function write_swift_object( $srcPath, $dstc, $dstRel) { |
| 356 | 356 | try { |
| 357 | 357 | $obj = $dstc->create_object($dstRel); |
| 358 | 358 | $obj->load_from_filename( $srcPath, True); |
| — | — | @@ -359,8 +359,8 @@ |
| 360 | 360 | throw new MWException( 'missing required parameters' ); |
| 361 | 361 | } catch (BadContentTypeException $e) { |
| 362 | 362 | throw new MWException( 'No Content-Type was/could be set' ); |
| 363 | | - } catch (InvalidResponseException $e) { |
| 364 | | - throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| | 363 | + #} catch (InvalidResponseException $e) { |
| | 364 | + # throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| 365 | 365 | } catch (IOException $e) { |
| 366 | 366 | throw new MWException( "error opening file '$e'" ); |
| 367 | 367 | } |
| — | — | @@ -372,15 +372,15 @@ |
| 373 | 373 | * an Internal Error on them. |
| 374 | 374 | * |
| 375 | 375 | */ |
| 376 | | - protected function swift_delete( $container, $rel ) { |
| | 376 | + function swift_delete( $container, $rel ) { |
| 377 | 377 | try { |
| 378 | 378 | $container->delete_object($rel); |
| 379 | 379 | } catch (SyntaxException $e) { |
| 380 | 380 | throw new MWException( "Swift object name not well-formed: '$e'" ); |
| 381 | 381 | } catch (NoSuchObjectException $e) { |
| 382 | 382 | throw new MWException( "Swift object we are trying to delete does not exist: '$e'" ); |
| 383 | | - } catch (InvalidResponseException $e) { |
| 384 | | - throw new MWException( "unexpected response '$e'" ); |
| | 383 | + #} catch (InvalidResponseException $e) { |
| | 384 | + # throw new MWException( "unexpected response '$e'" ); |
| 385 | 385 | } |
| 386 | 386 | } |
| 387 | 387 | |
| — | — | @@ -458,16 +458,9 @@ |
| 459 | 459 | unlink ( $srcPath ); |
| 460 | 460 | } |
| 461 | 461 | } |
| 462 | | - |
| 463 | | - if ( !( $flags & self::SKIP_VALIDATION ) ) { |
| 464 | | - // FIXME: Swift will return the MD5 of the data written. |
| 465 | | - if (0) { // ( $hashDest === false || $hashSource !== $hashDest ) |
| 466 | | - wfDebug( __METHOD__ . ': File copy validation failed: ' . |
| 467 | | - "$srcPath ($hashSource) to $dstPath ($hashDest)\n" ); |
| 468 | | - |
| 469 | | - $status->error( 'filecopyerror', $srcPath, $dstPath ); |
| 470 | | - $good = false; |
| 471 | | - } |
| | 462 | + if (0) { |
| | 463 | + $status->error( 'filecopyerror', $srcPath, $dstPath ); |
| | 464 | + $good = false; |
| 472 | 465 | } |
| 473 | 466 | if ( $good ) { |
| 474 | 467 | $status->successCount++; |
| — | — | @@ -630,8 +623,8 @@ |
| 631 | 624 | throw new MWException( "Missing Content-Type: $e" ); |
| 632 | 625 | } catch (MisMatchedChecksumException $e ) { |
| 633 | 626 | throw new MWException( __METHOD__ . "should not happen: '$e'" ); |
| 634 | | - } catch (InvalidResponseException $e ) { |
| 635 | | - throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| | 627 | + #} catch (InvalidResponseException $e ) { |
| | 628 | + # throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| 636 | 629 | } |
| 637 | 630 | |
| 638 | 631 | try { |
| — | — | @@ -648,8 +641,8 @@ |
| 649 | 642 | throw new MWException( 'Source file does not exist: ' . $srcContainer->name . "/$srcRel: $e" ); |
| 650 | 643 | } catch (MisMatchedChecksumException $e ) { |
| 651 | 644 | throw new MWException( "Checksums do not match: $e" ); |
| 652 | | - } catch (InvalidResponseException $e ) { |
| 653 | | - throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| | 645 | + #} catch (InvalidResponseException $e ) { |
| | 646 | + # throw new MWException( __METHOD__ . "unexpected response '$e'" ); |
| 654 | 647 | } |
| 655 | 648 | } |
| 656 | 649 | |
| — | — | @@ -678,18 +671,31 @@ |
| 679 | 672 | return $status; |
| 680 | 673 | } |
| 681 | 674 | |
| 682 | | - $conn = $this->connect(); |
| 683 | | - $container = $this->get_container($conn,$this->container); |
| | 675 | + try { |
| | 676 | + $conn = $this->connect(); |
| | 677 | + $container = $this->get_container($conn,$this->container); |
| | 678 | + } catch (InvalidResponseException $e) { |
| | 679 | + $status->fatal("Unexpected Swift response: '$e'"); |
| | 680 | + } |
| 684 | 681 | |
| | 682 | + if ( !$status->ok ) { |
| | 683 | + return $status; |
| | 684 | + } |
| | 685 | + |
| 685 | 686 | foreach ( $triplets as $i => $triplet ) { |
| 686 | 687 | list( $srcPath, $dstRel, $archiveRel ) = $triplet; |
| 687 | 688 | |
| 688 | 689 | // Archive destination file if it exists |
| 689 | 690 | try { |
| 690 | 691 | $pic = $container->get_object($dstRel); |
| | 692 | + } catch (InvalidResponseException $e) { |
| | 693 | + $status->error("Unexpected Swift response: '$e'"); |
| | 694 | + $status->failCount++; |
| | 695 | + continue; |
| 691 | 696 | } catch (NoSuchObjectException $e) { |
| 692 | 697 | $pic = NULL; |
| 693 | 698 | } |
| | 699 | + |
| 694 | 700 | if( $pic ) { |
| 695 | 701 | $this->swiftcopy($container, $dstRel, $container, $archiveRel ); |
| 696 | 702 | wfDebug(__METHOD__.": moved file $dstRel to $archiveRel\n"); |
| — | — | @@ -698,26 +704,30 @@ |
| 699 | 705 | $status->value[$i] = 'new'; |
| 700 | 706 | } |
| 701 | 707 | |
| 702 | | - // Where are we copying this from? |
| 703 | | - if (self::isVirtualUrl( $srcPath )) { |
| 704 | | - $src = $this->getContainerRel( $srcPath ); |
| 705 | | - list ($srcContainer, $srcRel) = $src; |
| 706 | | - $srcc = $this->get_container($conn, $srcContainer); |
| | 708 | + $good = true; |
| | 709 | + try { |
| | 710 | + // Where are we copying this from? |
| | 711 | + if (self::isVirtualUrl( $srcPath )) { |
| | 712 | + $src = $this->getContainerRel( $srcPath ); |
| | 713 | + list ($srcContainer, $srcRel) = $src; |
| | 714 | + $srcc = $this->get_container($conn, $srcContainer); |
| 707 | 715 | |
| 708 | | - $this->swiftcopy($srcc, $srcRel, $container, $dstRel); |
| 709 | | - if ( $flags & self::DELETE_SOURCE ) { |
| 710 | | - $this->swift_delete( $srcc, $srcRel ); |
| | 716 | + $this->swiftcopy($srcc, $srcRel, $container, $dstRel); |
| | 717 | + if ( $flags & self::DELETE_SOURCE ) { |
| | 718 | + $this->swift_delete( $srcc, $srcRel ); |
| | 719 | + } |
| | 720 | + } else { |
| | 721 | + $this->write_swift_object( $srcPath, $container, $dstRel); |
| | 722 | + // php-cloudfiles throws exceptions, so failure never gets here. |
| | 723 | + if ( $flags & self::DELETE_SOURCE ) { |
| | 724 | + unlink ( $srcPath ); |
| | 725 | + } |
| 711 | 726 | } |
| 712 | | - } else { |
| 713 | | - $this->write_swift_object( $srcPath, $container, $dstRel); |
| 714 | | - // php-cloudfiles throws exceptions, so failure never gets here. |
| 715 | | - if ( $flags & self::DELETE_SOURCE ) { |
| 716 | | - unlink ( $srcPath ); |
| 717 | | - } |
| | 727 | + } catch (InvalidResponseException $e) { |
| | 728 | + $status->error("Unexpected Swift response: '$e'"); |
| | 729 | + $good = false; |
| 718 | 730 | } |
| 719 | 731 | |
| 720 | | - $good = true; |
| 721 | | - |
| 722 | 732 | if ( $good ) { |
| 723 | 733 | $status->successCount++; |
| 724 | 734 | wfDebug(__METHOD__.": wrote tempfile $srcPath to $dstRel\n"); |
| — | — | @@ -884,8 +894,9 @@ |
| 885 | 895 | * copy of the file MUST delete the produced file, or else store it in |
| 886 | 896 | * SwiftFile->tempPath so it will be deleted when the object goes out of |
| 887 | 897 | * scope. |
| | 898 | + * FIXME: how do we return a fatal error from Swift? |
| 888 | 899 | */ |
| 889 | | - protected function getLocalCopy($container, $rel) { |
| | 900 | + function getLocalCopy($container, $rel) { |
| 890 | 901 | |
| 891 | 902 | // get a temporary place to put the original. |
| 892 | 903 | $tempPath = tempnam( wfTempDir(), 'swift_in_' ) . '.' . pathinfo( $rel, PATHINFO_EXTENSION ); |
| — | — | @@ -1270,15 +1281,16 @@ |
| 1271 | 1282 | } |
| 1272 | 1283 | |
| 1273 | 1284 | function getMasterDB() { |
| | 1285 | + wfDebug( __METHOD__.": {$this->dbServer}\n" ); |
| 1274 | 1286 | if ( !isset( $this->dbConn ) ) { |
| 1275 | | - $this->dbConn = DatabaseBase::newFromType( $this->dbType, |
| | 1287 | + $this->dbConn = DatabaseBase::factory( $this->dbType, |
| 1276 | 1288 | array( |
| 1277 | | - 'server' => $this->dbServer, |
| | 1289 | + 'host' => $this->dbServer, |
| 1278 | 1290 | 'user' => $this->dbUser, |
| 1279 | 1291 | 'password' => $this->dbPassword, |
| 1280 | 1292 | 'dbname' => $this->dbName, |
| 1281 | 1293 | 'flags' => $this->dbFlags, |
| 1282 | | - 'tableprefix' => $this->tablePrefix |
| | 1294 | + 'tablePrefix' => $this->tablePrefix |
| 1283 | 1295 | ) |
| 1284 | 1296 | ); |
| 1285 | 1297 | } |
| — | — | @@ -1331,7 +1343,7 @@ |
| 1332 | 1344 | * |
| 1333 | 1345 | * @ingroup FileRepo |
| 1334 | 1346 | */ |
| 1335 | | -class SwiftForeignDBViaLBRepo extends LocalRepo { |
| | 1347 | +class SwiftForeignDBViaLBRepo extends SwiftRepo{ |
| 1336 | 1348 | var $wiki, $dbName, $tablePrefix; |
| 1337 | 1349 | var $fileFactory = array( 'SwiftForeignDBFile', 'newFromTitle' ); |
| 1338 | 1350 | var $fileFromRowFactory = array( 'SwiftForeignDBFile', 'newFromRow' ); |