Revert/rewrite of r40696.
authorTim Starling <tstarling@users.mediawiki.org>
Sun, 21 Sep 2008 06:42:46 +0000 (06:42 +0000)
committerTim Starling <tstarling@users.mediawiki.org>
Sun, 21 Sep 2008 06:42:46 +0000 (06:42 +0000)
* We used to have parameters to ignore errors, but they're obsolete now that we have exceptions. Implemented ES master failover using exceptions instead.
* Changing the number of DB connection attempts from 3 to 2 for some random getConnection() calls is almost pointless, adds lots of ugly formal parameters all of the place, and misses the big picture. It should be 2 by default, based on the original rationale. Any reasonable implementation of failover should have zero timeouts per request, by storing state. Changed the default to 2, or 1 if a long timeout is set.

includes/ExternalStore.php
includes/ExternalStoreDB.php
includes/db/Database.php
includes/db/LoadBalancer.php

index e73b9a1..34570e2 100644 (file)
@@ -77,30 +77,36 @@ class ExternalStore {
         */
        public static function randomInsert( $data ) {
                global $wgDefaultExternalStore;
-               $tryStorages = (array)$wgDefaultExternalStore;
-               // Do not wait and do second retry per master if we
-               // have other active cluster masters to try instead.
-               $retry = count($tryStorages) > 1 ? false : true;
-               while ( count($tryStorages) > 0 ) {
-                       $index = mt_rand(0, count( $tryStorages ) - 1);
-                       $storeUrl = $tryStorages[$index];
+               $tryStores = (array)$wgDefaultExternalStore;
+               $error = false;
+               while ( count( $tryStores ) > 0 ) {
+                       $index = mt_rand(0, count( $tryStores ) - 1);
+                       $storeUrl = $tryStores[$index];
+                       wfDebug( __METHOD__.": trying $storeUrl\n" );
                        list( $proto, $params ) = explode( '://', $storeUrl, 2 );
                        $store = self::getStoreObject( $proto );
                        if ( $store === false ) {
                                throw new MWException( "Invalid external storage protocol - $storeUrl" );
-                               return false; 
+                       }
+                       try {
+                               $url = $store->store( $params, $data ); // Try to save the object
+                       } catch ( DBConnectionError $error ) {
+                               $url = false;
+                               unset( $tryStores[$index] ); // Don't try this one again!
+                               $tryStores = array_values( $tryStores ); // Must have consecutive keys
+                       }
+                       if ( $url ) {
+                               return $url; // Done!
                        } else {
-                               $url = $store->store( $params, $data, $retry ); // Try to save the object
-                               if ( $url ) {
-                                       return $url; // Done!
-                               } else {
-                                       unset( $tryStorages[$index] ); // Don't try this one again!
-                                       sort( $tryStorages ); // Must have consecutive keys
-                                       wfDebugLog( 'ExternalStorage', "Unable to store text to external storage $storeUrl" );
-                               }
+                               wfDebugLog( 'ExternalStorage', "Unable to store text to external storage $storeUrl" );
                        }
                }
-               throw new MWException( "Unable to store text to external storage" );
-               return false; // All cluster masters dead :(
+               // All stores failed
+               if ( $error ) {
+                       // Rethrow the last connection error
+                       throw $error;
+               } else {
+                       throw new MWException( "Unable to store text to external storage" );
+               }
        }
 }
index fb3853a..caac3b1 100644 (file)
@@ -34,14 +34,13 @@ class ExternalStoreDB {
        /** @todo Document.*/
        function &getSlave( $cluster ) {
                $lb =& $this->getLoadBalancer( $cluster );
-               // Make only two connection attempts, since we still have the master to try
-               return $lb->getConnection( DB_SLAVE, array(), false, 2 );
+               return $lb->getConnection( DB_SLAVE );
        }
 
        /** @todo Document.*/
-       function &getMaster( $cluster, $retry = true ) {
+       function &getMaster( $cluster ) {
                $lb =& $this->getLoadBalancer( $cluster );
-               return $lb->getConnection( DB_MASTER, array(), false, false, LoadBalancer::GRACEFUL );
+               return $lb->getConnection( DB_MASTER, array() );
        }
 
        /** @todo Document.*/
@@ -120,13 +119,11 @@ class ExternalStoreDB {
         *
         * @param $cluster String: the cluster name
         * @param $data String: the data item
-        * @param $retry bool: allows an extra DB connection retry after 1 second
         * @return string URL
         */
-       function store( $cluster, $data, $retry = true ) {
-               if( !$dbw = $this->getMaster( $cluster, $retry ) ) {
-                       return false; // failed, maybe another cluster is up...
-               }
+       function store( $cluster, $data ) {
+               $dbw = $this->getMaster( $cluster );
+
                $id = $dbw->nextSequenceValue( 'blob_blob_id_seq' );
                $dbw->insert( $this->getTable( $dbw ), 
                        array( 'blob_id' => $id, 'blob_text' => $data ), 
index 476260d..83cf368 100644 (file)
@@ -257,11 +257,9 @@ class Database {
         * @param failFunction
         * @param $flags
         * @param $tablePrefix String: database table prefixes. By default use the prefix gave in LocalSettings.php
-        * @param int $max, max connection attempts 
-        ** After the first retry (second attempt), each retry waits 1 second
         */
        function __construct( $server = false, $user = false, $password = false, $dbName = false,
-               $failFunction = false, $flags = 0, $tablePrefix = 'get from global', $max = false ) {
+               $failFunction = false, $flags = 0, $tablePrefix = 'get from global' ) {
 
                global $wgOut, $wgDBprefix, $wgCommandLineMode;
                # Can't get a reference if it hasn't been set yet
@@ -295,7 +293,7 @@ class Database {
                }
 
                if ( $server ) {
-                       $this->open( $server, $user, $password, $dbName, $max );
+                       $this->open( $server, $user, $password, $dbName );
                }
        }
 
@@ -313,7 +311,7 @@ class Database {
         * Usually aborts on failure
         * If the failFunction is set to a non-zero integer, returns success
         */
-       function open( $server, $user, $password, $dbName, $max = false ) {
+       function open( $server, $user, $password, $dbName ) {
                global $wguname, $wgAllDBsAreLocalhost;
                wfProfileIn( __METHOD__ );
 
@@ -345,13 +343,17 @@ class Database {
 
                wfProfileIn("dbconnect-$server");
 
-               if( !$max ) { $max = 3; }
-               # Try to connect up to three times (by default)
                # The kernel's default SYN retransmission period is far too slow for us,
-               # so we use a short timeout plus a manual retry.
+               # so we use a short timeout plus a manual retry. Retrying means that a small
+               # but finite rate of SYN packet loss won't cause user-visible errors.
                $this->mConn = false;
+               if ( ini_get( 'mysql.connect_timeout' ) <= 3 ) {
+                       $numAttempts = 2;
+               } else {
+                       $numAttempts = 1;
+               }
                $this->installErrorHandler();
-               for ( $i = 0; $i < $max && !$this->mConn; $i++ ) {
+               for ( $i = 0; $i < $numAttempts && !$this->mConn; $i++ ) {
                        if ( $i > 1 ) {
                                usleep( 1000 );
                        }
@@ -367,6 +369,7 @@ class Database {
                        }
                }
                $phpError = $this->restoreErrorHandler();
+               # Always log connection errors
                if ( !$this->mConn ) {
                        $error = $this->lastError();
                        if ( !$error ) {
@@ -2507,6 +2510,13 @@ border=\"0\" ALT=\"Google\"></A>
 
                $text = str_replace( '$1', $this->error, $noconnect );
 
+               global $wgShowExceptionDetails;
+               if ( $GLOBALS['wgShowExceptionDetails'] ) {
+                       $text .= '</p><p>Backtrace:</p><p>' . 
+                               nl2br( htmlspecialchars( $this->getTraceAsString() ) ) . 
+                               "</p>\n";
+               }
+
                if($wgUseFileCache) {
                        if($wgTitle) {
                                $t =& $wgTitle;
index 733ee5e..cfea8b3 100644 (file)
@@ -11,8 +11,6 @@
  * @ingroup Database
  */
 class LoadBalancer {
-       const GRACEFUL = 1;
-
        /* private */ var $mServers, $mConns, $mLoads, $mGroupLoads;
        /* private */ var $mFailFunction, $mErrorConnection;
        /* private */ var $mReadIndex, $mAllowLagged;
@@ -172,7 +170,7 @@ class LoadBalancer {
         *
         * Side effect: opens connections to databases
         */
-       function getReaderIndex( $group = false, $wiki = false, $attempts = false ) {
+       function getReaderIndex( $group = false, $wiki = false ) {
                global $wgReadOnly, $wgDBClusterTimeout, $wgDBAvgStatusPoll, $wgDBtype;
 
                # FIXME: For now, only go through all this for mysql databases
@@ -249,7 +247,7 @@ class LoadBalancer {
                                }
 
                                wfDebugLog( 'connect', __METHOD__.": Using reader #$i: {$this->mServers[$i]['host']}...\n" );
-                               $conn = $this->openConnection( $i, $wiki, $attempts );
+                               $conn = $this->openConnection( $i, $wiki );
 
                                if ( !$conn ) {
                                        wfDebugLog( 'connect', __METHOD__.": Failed connecting to $i/$wiki\n" );
@@ -403,10 +401,8 @@ class LoadBalancer {
         * @param int $i Database
         * @param array $groups Query groups
         * @param string $wiki Wiki ID
-        * @param int $attempts Max DB connect attempts
-        * @param int $flags
         */
-       public function &getConnection( $i, $groups = array(), $wiki = false, $attempts = false, $flags = 0 ) {
+       public function &getConnection( $i, $groups = array(), $wiki = false ) {
                global $wgDBtype;
                wfProfileIn( __METHOD__ );
 
@@ -438,21 +434,18 @@ class LoadBalancer {
 
                # Operation-based index
                if ( $i == DB_SLAVE ) {
-                       $i = $this->getReaderIndex( false, $wiki, $attempts );
+                       $i = $this->getReaderIndex( false, $wiki );
                } elseif ( $i == DB_LAST ) {
                        throw new MWException( 'Attempt to call ' . __METHOD__ . ' with deprecated server index DB_LAST' );
                }
                # Couldn't find a working server in getReaderIndex()?
                if ( $i === false ) {
-                       if( $flags && self::GRACEFUL ) {
-                               return false;
-                       }
                        $this->reportConnectionError( $this->mErrorConnection );
                }
 
                # Now we have an explicit index into the servers array
-               $conn = $this->openConnection( $i, $wiki, $attempts );
-               if ( !$conn && !($flags & self::GRACEFUL) ) {
+               $conn = $this->openConnection( $i, $wiki );
+               if ( !$conn ) {
                        $this->reportConnectionError( $this->mErrorConnection );
                }
 
@@ -512,12 +505,11 @@ class LoadBalancer {
         *
         * @param integer $i Server index
         * @param string $wiki Wiki ID to open
-        * @param int $attempts Maximum connection attempts
         * @return Database
         *
         * @access private
         */
-       function openConnection( $i, $wiki = false, $attempts = false ) {
+       function openConnection( $i, $wiki = false ) {
                wfProfileIn( __METHOD__ );
                if ( $wiki !== false ) {
                        $conn = $this->openForeignConnection( $i, $wiki );
@@ -529,7 +521,7 @@ class LoadBalancer {
                } else {
                        $server = $this->mServers[$i];
                        $server['serverIndex'] = $i;
-                       $conn = $this->reallyOpenConnection( $server, false, $attempts );
+                       $conn = $this->reallyOpenConnection( $server, false );
                        if ( $conn->isOpen() ) {
                                $this->mConns['local'][$i][0] = $conn;
                        } else {
@@ -631,7 +623,7 @@ class LoadBalancer {
         * Returns a Database object whether or not the connection was successful.
         * @access private
         */
-       function reallyOpenConnection( $server, $dbNameOverride = false, $attempts = false ) {
+       function reallyOpenConnection( $server, $dbNameOverride = false ) {
                if( !is_array( $server ) ) {
                        throw new MWException( 'You must update your load-balancing configuration. See DefaultSettings.php entry for $wgDBservers.' );
                }
@@ -646,7 +638,7 @@ class LoadBalancer {
 
                # Create object
                wfDebug( "Connecting to $host $dbname...\n" );
-               $db = new $class( $host, $user, $password, $dbname, 1, $flags, 'get from global', $attempts );
+               $db = new $class( $host, $user, $password, $dbname, 1, $flags );
                if ( $db->isOpen() ) {
                        wfDebug( "Connected\n" );
                } else {