jenkins-bot has submitted this change and it was merged.

Change subject: Database class parameter and documentation cleanups
......................................................................


Database class parameter and documentation cleanups

* Document various parameter arrays.
* Fix $user = false loophole in Database::__construct().
* Set the Postgres port *before* calling super, as it is
  needed by open().
* Remove 'chronProt' parameter as it is lazy-loaded.

Change-Id: Icc1037efa1eee7ae6fdd2919f60001e6e29ae55c
---
M includes/libs/rdbms/database/Database.php
M includes/libs/rdbms/database/DatabasePostgres.php
M includes/libs/rdbms/database/DatabaseSqlite.php
M includes/libs/rdbms/lbfactory/LBFactory.php
M includes/libs/rdbms/lbfactory/LBFactoryMulti.php
M includes/libs/rdbms/lbfactory/LBFactorySimple.php
M includes/libs/rdbms/loadbalancer/ILoadBalancer.php
7 files changed, 156 insertions(+), 94 deletions(-)

Approvals:
  Tim Starling: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/libs/rdbms/database/Database.php 
b/includes/libs/rdbms/database/Database.php
index f9e9296..a5b9284 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -231,16 +231,12 @@
        protected $trxProfiler;
 
        /**
-        * Constructor.
-        *
-        * FIXME: It is possible to construct a Database object with no 
associated
-        * connection object, by specifying no parameters to __construct(). This
-        * feature is deprecated and should be removed.
+        * Constructor and database handle and attempt to connect to the DB 
server
         *
         * IDatabase classes should not be constructed directly in external
-        * code. DatabaseBase::factory() should be used instead.
+        * code. Database::factory() should be used instead.
         *
-        * @param array $params Parameters passed from DatabaseBase::factory()
+        * @param array $params Parameters passed from Database::factory()
         */
        function __construct( array $params ) {
                $server = $params['host'];
@@ -287,37 +283,59 @@
 
                if ( $user ) {
                        $this->open( $server, $user, $password, $dbName );
+               } elseif ( $this->requiresDatabaseUser() ) {
+                       throw new InvalidArgumentException( "No database user 
provided." );
                }
 
+               // Set the domain object after open() sets the relevant fields
                $this->currentDomain = ( $this->mDBname != '' )
                        ? new DatabaseDomain( $this->mDBname, null, 
$this->mTablePrefix )
                        : DatabaseDomain::newUnspecified();
        }
 
        /**
-        * Given a DB type, construct the name of the appropriate child class of
-        * IDatabase. This is designed to replace all of the manual stuff like:
-        *    $class = 'Database' . ucfirst( strtolower( $dbType ) );
-        * as well as validate against the canonical list of DB types we have
+        * Construct a Database subclass instance given a database type and 
parameters
         *
-        * This factory function is mostly useful for when you need to connect 
to a
-        * database other than the MediaWiki default (such as for external auth,
-        * an extension, et cetera). Do not use this to connect to the MediaWiki
-        * database. Example uses in core:
-        * @see LoadBalancer::reallyOpenConnection()
-        * @see ForeignDBRepo::getMasterDB()
-        * @see WebInstallerDBConnect::execute()
+        * This also connects to the database immediately upon object 
construction
         *
-        * @since 1.18
-        *
-        * @param string $dbType A possible DB type
-        * @param array $p An array of options to pass to the constructor.
-        *    Valid options are: host, user, password, dbname, flags, 
tablePrefix, schema, driver
-        * @return IDatabase|null If the database driver or extension cannot be 
found
+        * @param string $dbType A possible DB type (sqlite, mysql, postgres)
+        * @param array $p Parameter map with keys:
+        *   - host : The hostname of the DB server
+        *   - user : The name of the database user the client operates under
+        *   - password : The password for the database user
+        *   - dbname : The name of the database to use where queries do not 
specify one.
+        *      The database must exist or an error might be thrown. Setting 
this to the empty string
+        *      will avoid any such errors and make the handle have no implicit 
database scope. This is
+        *      useful for queries like SHOW STATUS, CREATE DATABASE, or DROP 
DATABASE. Note that a
+        *      "database" in Postgres is rougly equivalent to an entire MySQL 
server. This the domain
+        *      in which user names and such are defined, e.g. users are 
database-specific in Postgres.
+        *   - schema : The database schema to use (if supported). A "schema" 
in Postgres is roughly
+        *      equivalent to a "database" in MySQL. Note that MySQL and SQLite 
do not use schemas.
+        *   - tablePrefix : Optional table prefix that is implicitly added on 
to all table names
+        *      recognized in queries. This can be used in place of schemas for 
handle site farms.
+        *   - flags : Optional bitfield of DBO_* constants that define 
connection, protocol,
+        *      buffering, and transaction behavior. It is STRONGLY adviced to 
leave the DBO_DEFAULT
+        *      flag in place UNLESS this this database simply acts as a 
key/value store.
+        *   - driver: Optional name of a specific DB client driver. For MySQL, 
there is the old
+        *      'mysql' driver and the newer 'mysqli' driver.
+        *   - variables: Optional map of session variables to set after 
connecting. This can be
+        *      used to adjust lock timeouts or encoding modes and the like.
+        *   - connLogger: Optional PSR-3 logger interface instance.
+        *   - queryLogger: Optional PSR-3 logger interface instance.
+        *   - profiler: Optional class name or object with 
profileIn()/profileOut() methods.
+        *      These will be called in query(), using a simplified version of 
the SQL that also
+        *      includes the agent as a SQL comment.
+        *   - trxProfiler: Optional TransactionProfiler instance.
+        *   - errorLogger: Optional callback that takes an Exception and logs 
it.
+        *   - cliMode: Whether to consider the execution context that of a CLI 
script.
+        *   - agent: Optional name used to identify the end-user in query 
profiling/logging.
+        *   - srvCache: Optional BagOStuff instance to an APC-style cache.
+        * @return Database|null If the database driver or extension cannot be 
found
         * @throws InvalidArgumentException If the database driver or extension 
cannot be found
+        * @since 1.18
         */
        final public static function factory( $dbType, $p = [] ) {
-               $canonicalDBTypes = [
+               static $canonicalDBTypes = [
                        'mysql' => [ 'mysqli', 'mysql' ],
                        'postgres' => [],
                        'sqlite' => [],
@@ -667,7 +685,7 @@
        }
 
        /**
-        * Create a log context to pass to PSR logging functions.
+        * Create a log context to pass to PSR-3 logger functions.
         *
         * @param array $extras Additional data to add to context
         * @return array
@@ -3457,6 +3475,14 @@
        }
 
        /**
+        * @return bool Whether a DB user is required to access the DB
+        * @since 1.28
+        */
+       protected function requiresDatabaseUser() {
+               return true;
+       }
+
+       /**
         * @since 1.19
         * @return string
         */
diff --git a/includes/libs/rdbms/database/DatabasePostgres.php 
b/includes/libs/rdbms/database/DatabasePostgres.php
index b07ac16..e79b28b 100644
--- a/includes/libs/rdbms/database/DatabasePostgres.php
+++ b/includes/libs/rdbms/database/DatabasePostgres.php
@@ -43,8 +43,8 @@
        private $mCoreSchema;
 
        public function __construct( array $params ) {
-               parent::__construct( $params );
                $this->port = isset( $params['port'] ) ? $params['port'] : 
false;
+               parent::__construct( $params );
        }
 
        function getType() {
diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php 
b/includes/libs/rdbms/database/DatabaseSqlite.php
index 8881983..6614898 100644
--- a/includes/libs/rdbms/database/DatabaseSqlite.php
+++ b/includes/libs/rdbms/database/DatabaseSqlite.php
@@ -1046,11 +1046,14 @@
                return $this->query( $sql, $fName );
        }
 
+       protected function requiresDatabaseUser() {
+               return false; // just a file
+       }
+
        /**
         * @return string
         */
        public function __toString() {
                return 'SQLite ' . (string)$this->mConn->getAttribute( 
PDO::ATTR_SERVER_VERSION );
        }
-
 }
diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php 
b/includes/libs/rdbms/lbfactory/LBFactory.php
index 40ba458..38132c7 100644
--- a/includes/libs/rdbms/lbfactory/LBFactory.php
+++ b/includes/libs/rdbms/lbfactory/LBFactory.php
@@ -80,8 +80,26 @@
                [ 'replLogger', 'connLogger', 'queryLogger', 'perfLogger' ];
 
        /**
-        * @TODO: document base params here
-        * @param array $conf
+        * Construct a manager of ILoadBalancer objects
+        *
+        * Sub-classes will extend the required keys in $conf with additional 
parameters
+        *
+        * @param $conf $params Array with keys:
+        *  - localDomain: A DatabaseDomain or domain ID string.
+        *  - readOnlyReason : Reason the master DB is read-only if so 
[optional]
+        *  - srvCache : BagOStuff object for server cache [optional]
+        *  - memCache : BagOStuff object for cluster memory cache [optional]
+        *  - wanCache : WANObjectCache object [optional]
+        *  - hostname : The name of the current server [optional]
+        *  - cliMode: Whether the execution context is a CLI script. [optional]
+        *  - profiler : Class name or instance with profileIn()/profileOut() 
methods. [optional]
+        *  - trxProfiler: TransactionProfiler instance. [optional]
+        *  - replLogger: PSR-3 logger instance. [optional]
+        *  - connLogger: PSR-3 logger instance. [optional]
+        *  - queryLogger: PSR-3 logger instance. [optional]
+        *  - perfLogger: PSR-3 logger instance. [optional]
+        *  - errorLogger : Callback that takes an Exception and logs it. 
[optional]
+        * @throws InvalidArgumentException
         */
        public function __construct( array $conf ) {
                $this->localDomain = isset( $conf['localDomain'] )
@@ -106,8 +124,6 @@
                        : function ( Exception $e ) {
                                trigger_error( E_WARNING, get_class( $e ) . ': 
' . $e->getMessage() );
                        };
-
-               $this->chronProt = isset( $conf['chronProt'] ) ? 
$conf['chronProt'] : null;
 
                $this->profiler = isset( $params['profiler'] ) ? 
$params['profiler'] : null;
                $this->trxProfiler = isset( $conf['trxProfiler'] )
diff --git a/includes/libs/rdbms/lbfactory/LBFactoryMulti.php 
b/includes/libs/rdbms/lbfactory/LBFactoryMulti.php
index 0f1493a..25e1fe0 100644
--- a/includes/libs/rdbms/lbfactory/LBFactoryMulti.php
+++ b/includes/libs/rdbms/lbfactory/LBFactoryMulti.php
@@ -25,62 +25,6 @@
  * A multi-database, multi-master factory for Wikimedia and similar 
installations.
  * Ignores the old configuration globals.
  *
- * Template override precedence (highest => lowest):
- *   - templateOverridesByServer
- *   - masterTemplateOverrides
- *   - templateOverridesBySection/templateOverridesByCluster
- *   - externalTemplateOverrides
- *   - serverTemplate
- * Overrides only work on top level keys (so nested values will not be merged).
- *
- * Configuration:
- *     sectionsByDB                A map of database names to section names.
- *
- *     sectionLoads                A 2-d map. For each section, gives a map of 
server names to
- *                                 load ratios. For example:
- *                                 [
- *                                     'section1' => [
- *                                         'db1' => 100,
- *                                         'db2' => 100
- *                                     ]
- *                                 ]
- *
- *     serverTemplate              A server info associative array as 
documented for $wgDBservers.
- *                                 The host, hostName and load entries will be 
overridden.
- *
- *     groupLoadsBySection         A 3-d map giving server load ratios for 
each section and group.
- *                                 For example:
- *                                 [
- *                                     'section1' => [
- *                                         'group1' => [
- *                                             'db1' => 100,
- *                                             'db2' => 100
- *                                         ]
- *                                     ]
- *                                 ]
- *
- *     groupLoadsByDB              A 3-d map giving server load ratios by DB 
name.
- *
- *     hostsByName                 A map of hostname to IP address.
- *
- *     externalLoads               A map of external storage cluster name to 
server load map.
- *
- *     externalTemplateOverrides   A set of server info keys overriding 
serverTemplate for external
- *                                 storage.
- *
- *     templateOverridesByServer   A 2-d map overriding serverTemplate and
- *                                 externalTemplateOverrides on a 
server-by-server basis. Applies
- *                                 to both core and external storage.
- *     templateOverridesBySection  A 2-d map overriding the server info by 
section.
- *     templateOverridesByCluster  A 2-d map overriding the server info by 
external storage cluster.
- *
- *     masterTemplateOverrides     An override array for all master servers.
- *
- *     loadMonitorClass            Name of the LoadMonitor class to always use.
- *
- *     readOnlyBySection           A map of section name to read-only message.
- *                                 Missing or false for read/write.
- *
  * @ingroup Database
  */
 class LBFactoryMulti extends LBFactory {
@@ -141,8 +85,6 @@
         */
        private $readOnlyBySection = [];
 
-       // Other stuff
-
        /** @var array Load balancer factory configuration */
        private $conf;
 
@@ -162,8 +104,60 @@
        private $lastSection;
 
        /**
-        * @param array $conf
-        * @throws InvalidArgumentException
+        * @see LBFactory::__construct()
+        *
+        * Template override precedence (highest => lowest):
+        *   - templateOverridesByServer
+        *   - masterTemplateOverrides
+        *   - templateOverridesBySection/templateOverridesByCluster
+        *   - externalTemplateOverrides
+        *   - serverTemplate
+        * Overrides only work on top level keys (so nested values will not be 
merged).
+        *
+        * Server configuration maps should be of the format 
Database::factory() requires.
+        * Additionally, a 'max lag' key should also be set on server maps, 
indicating how stale the
+        * data can be before the load balancer tries to avoid using it. The 
map can have 'is static'
+        * set to disable blocking  replication sync checks (intended for 
archive servers with
+        * unchanging data).
+        *
+        * @param array $conf Parameters of LBFactory::__construct() as well as:
+        *   - sectionsByDB                Map of database names to section 
names.
+        *   - sectionLoads                2-d map. For each section, gives a 
map of server names to
+        *                                 load ratios. For example:
+        *                                 [
+        *                                     'section1' => [
+        *                                         'db1' => 100,
+        *                                         'db2' => 100
+        *                                     ]
+        *                                 ]
+        *   - serverTemplate              Server configuration map intended 
for Database::factory().
+        *                                 Note that "host", "hostName" and 
"load" entries will be
+        *                                 overridden by "sectionLoads" and 
"hostsByName".
+        *   - groupLoadsBySection         3-d map giving server load ratios 
for each section/group.
+        *                                 For example:
+        *                                 [
+        *                                     'section1' => [
+        *                                         'group1' => [
+        *                                             'db1' => 100,
+        *                                             'db2' => 100
+        *                                         ]
+        *                                     ]
+        *                                 ]
+        *   - groupLoadsByDB              3-d map giving server load ratios by 
DB name.
+        *   - hostsByName                 Map of hostname to IP address.
+        *   - externalLoads               Map of external storage cluster name 
to server load map.
+        *   - externalTemplateOverrides   Set of server configuration maps 
overriding
+        *                                 "serverTemplate" for external 
storage.
+        *   - templateOverridesByServer   2-d map overriding "serverTemplate" 
and
+        *                                 "externalTemplateOverrides" on a 
server-by-server basis.
+        *                                 Applies to both core and external 
storage.
+        *   - templateOverridesBySection  2-d map overriding the server 
configuration maps by section.
+        *   - templateOverridesByCluster  2-d map overriding the server 
configuration maps by external
+        *                                 storage cluster.
+        *   - masterTemplateOverrides     Server configuration map overrides 
for all master servers.
+        *   - loadMonitorClass            Name of the LoadMonitor class to 
always use.
+        *   - readOnlyBySection           A map of section name to read-only 
message.
+        *                                 Missing or false for read/write.
         */
        public function __construct( array $conf ) {
                parent::__construct( $conf );
diff --git a/includes/libs/rdbms/lbfactory/LBFactorySimple.php 
b/includes/libs/rdbms/lbfactory/LBFactorySimple.php
index 0949092..b90afe6 100644
--- a/includes/libs/rdbms/lbfactory/LBFactorySimple.php
+++ b/includes/libs/rdbms/lbfactory/LBFactorySimple.php
@@ -38,6 +38,18 @@
        /** @var string */
        private $loadMonitorClass;
 
+       /**
+        * @see LBFactory::__construct()
+        * @param array $conf Parameters of LBFactory::__construct() as well as:
+        *   - servers : list of server configuration maps to 
Database::factory().
+        *      Additionally, the server maps should have a 'load' key, which 
is used to decide
+        *      how often clients connect to one server verses the others. A 
'max lag' key should
+        *      also be set on server maps, indicating how stale the data can 
be before the load
+        *      balancer tries to avoid using it. The map can have 'is static' 
set to disable blocking
+        *      replication sync checks (intended for archive servers with 
unchanging data).
+        *   - externalClusters : map of cluster names to server arrays. The 
servers arrays have the
+        *      same format as "servers" above.
+        */
        public function __construct( array $conf ) {
                parent::__construct( $conf );
 
diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php 
b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
index 0f6bea3..cdbfc77 100644
--- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php
@@ -30,15 +30,26 @@
  */
 interface ILoadBalancer {
        /**
-        * @param array $params Array with keys:
+        * Construct a manager of IDatabase connection objects
+        *
+        * @param array $params Parameter map with keys:
         *  - servers : Required. Array of server info structures.
+        *  - localDomain: A DatabaseDomain or domain ID string.
         *  - loadMonitor : Name of a class used to fetch server lag and load.
         *  - readOnlyReason : Reason the master DB is read-only if so 
[optional]
         *  - waitTimeout : Maximum time to wait for replicas for consistency 
[optional]
         *  - srvCache : BagOStuff object for server cache [optional]
         *  - memCache : BagOStuff object for cluster memory cache [optional]
         *  - wanCache : WANObjectCache object [optional]
-        *  - hostname : the name of the current server [optional]
+        *  - hostname : The name of the current server [optional]
+        *  - cliMode: Whether the execution context is a CLI script. [optional]
+        *  - profiler : Class name or instance with profileIn()/profileOut() 
methods. [optional]
+        *  - trxProfiler: TransactionProfiler instance. [optional]
+        *  - replLogger: PSR-3 logger instance. [optional]
+        *  - connLogger: PSR-3 logger instance. [optional]
+        *  - queryLogger: PSR-3 logger instance. [optional]
+        *  - perfLogger: PSR-3 logger instance. [optional]
+        *  - errorLogger : Callback that takes an Exception and logs it. 
[optional]
         * @throws InvalidArgumentException
         */
        public function __construct( array $params );

-- 
To view, visit https://gerrit.wikimedia.org/r/311629
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Icc1037efa1eee7ae6fdd2919f60001e6e29ae55c
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: MaxSem <maxsem.w...@gmail.com>
Gerrit-Reviewer: Tim Starling <tstarl...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to