Subclasses no longer have to implement fetchAffectedRowCount()
and affectedRows() no longer depends on the driver connection
handle.
Set "port" to $wgDBport in LBFactoryTest/LoadBalancerTest to
avoid postgres failures.
Bug: T314100
Change-Id: Ib31a9d2db18d7ba7dcf61fb110d0fef53f455464
There are many, many more places that can benefit from these new
interfaces. I like to go step by step. This makes patches like this
much easier to review.
Change-Id: I461064c1a7f70b3ba3b2a6d47a67cb6e8a54528d
Follows-up I4c7d826c7ec654b, I1287f3979aba1bf1.
We lose useful coverage and spend valuable time keeping these accurate
through refactors (or worse, forget to do so). The theoretically "bad"
accidental coverage is almost never actually bad.
Having said that, I'm not removing them wholesale (yet). I've audited
each of these specific files to confirm it is a general test of the
specified subject class, and also kept it limited to those specified
classes. That's imho more than 100% of the benefit for less than 1%
of the cost (more because `@covers` is more valuable than the fragile
and corrosive individual private method tracking in tests that
inevitably get out of date with no local incentive to keep them up to
date).
Cases like structure tests keep `@coversNothing` etc and we still don't
count coverage of other classes. There may be a handful of large
legacy classes where some methods are effectively class-like in
complexity and that's why it's good for PHPUnit to offer the precision
instrument but that doesn't meant we have to use that by-default for
everything.
I think best practice is to write good narrow unit tests, that reflect
how the code should be used in practice. Not to write bad tests and
hide part of its coverage within the same class or even namespace.
Fortunately, that's generally what we do already it's just that we
also kept these annotations still in many cases.
This wastes time to keep methods in sync, time to realize (and fix)
when other people inevitably didn't keep them in sync, time to find
uncovered code only to realize it is already covered, time for a less
experienced engineer to feel obligate to and do write a low quality
test to cover the "missing" branch in an unrealistic way, time wasted
in on-boarding by using such "bad" tests as example for how to use
the code and then having to unlearn it months/years later, loss of
telemetry in knowing what code actually isn't propertly tested due to
being masked by a bad test, and lost oppertunities to find actually
ununused/unreachable code and to think about how to instead structure
the code such that maybe that code can be removed.
------
Especially cases like LBFactoryTest.php were getting out of hand,
and in GlobalIdGeneratorTest.php we even resorted to reminding people
with inline comments to keep tags in sync.
Change-Id: I69b5385868cc6b451e5f2ebec9539694968bf58c
Use server names to handle the case where server indexes get
shifted around due to the depooling or provisioning of a server.
Previously, loads could be assigned to a wrong server, assigned
to a server that was depooled, or assigned to a server that is
to new to appear in the current "servers" array. This would mean
that getReaderIndex() or getLagTimes() could trigger exceptions.
Only change loads for depooled servers. Update the loads of other
servers to the current loads only makes sense in the context of
using all the current servers. This does not apply to reconfigure()
since it does not see newly pooled servers (for simplicity).
Handle servers depooled only from custom query groups.
Bug: T322156
Change-Id: I9f710aa32f5d5b74796bb80a8426a5f653b8e4d3
This was mentioned in the constructor comments but was not actually
implemented. It is now possible to defined what section is the catch-all
section, instead of it always being named "DEFAULT". Configuration of the
sections themselves can then be simplified by only using the real section
name for all sections.
For example, at Wikimedia, the catch-all section is "s3". The etcd config
uses "s3". The pt-heartbeat service uses "s3", thus "lagDetectionOptions"
must also use "s3". There is no reason that "templateOverridesBySection"
or "readOnlyBySection" should have to use "DEFAULT" as the key for "s3".
Change-Id: I098afd5f6108f5d7099a4cd6a611c5647ae96879
In LoadBalancer:
* Make the "chronologyCallback" return the DBPrimaryPos and make
loadSessionPrimaryPos() set the "waitForPos" more directly by
calling setSessionPrimaryPosIfHigher(). Previously, it relied
on the callback calling waitFor() to set the position as a side
effect.
* Remove redundant debug log entry in loadSessionPrimaryPos().
* Use type hints for waitFor()/waitForAll(). All callers already check
this for before invocation.
* Mark getReplicaResumePos() as @internal.
In ChronologyProtector:
* Update applySessionReplicationPosition() to return the position.
* Rename applySessionReplicationPosition() to yieldSessionPrimaryPos()
and stageSessionReplicationPosition() to stageSessionPrimaryPos() for
for consistency LoadBalancer/DBPrimaryPos.
Bug: T314434
Change-Id: I32aa784b424e7534047c9240e32fa5e0a2ac90b0
Factor out normalizeServerMaps() method to simplify getServerName()
by pre-setting "serverName". Add uniqueness safety check, since we
rely on this property in cache keys and in reconfigure().
Change-Id: I06672885c79611e1257adb5f3dec88194e71b705
Notable changes:
* In SqlBagOStuff::getConnectionFromServerInfo, only two loggers were
injected. The rest implicitly got a NullLogger due to being absent.
These are now effectively unsilenced.
* Database::__construct() required almost all parameters, even the
loggers. I've wanted to move some of DatabaseFactory into the ctor
here for a while. In order to make this change not a breaking
change, the new 'logger' parameter is optional with NullLogger as
default. This allowed some of the test cases, which were simply
passing NullLogger, to be fixed by passing nothing instead of
passing the new option name.
The Database class is behind a dozen layers of indirection for
real use, so this will still be injected just fine (DBF, LB, LBF,
MWLBF, etc.).
* In LegacyLogger, the handling for $wgDBerrorLog was previously
limited to DBConnection and DBQuery. This now includes errors
from other (generally, newer) parts of Rdbms as well, which were
previously missing.
This only affects sites (typically CI and dev setup) where
$wgDBerrorLog is used, as opposed to the more common
$wgDebugLogGroups by-channel configuration.
* TransactionProfiler gets its logger injected in a rather odd way,
via entrypoints (MediaWiki.php, ApiMain.php, and MaintenanceRunner)
as opposed to service wiring. This is kept as-is for now.
* In LBFactoryTest, in particular testInvalidSelectDBIndependent2,
there are cases that intentionally produce failures of which
the result is then observed. In CI we assert that dberror.log is
empty so instead of adding the missing logger fields to that
LBFactory instance, the only one set (replLogger) is removed.
The alternative is to set 'logger' now, which would naturally
cause CI failures due to unexpected entries coming through to
non-mocked error log.
Bug: T320873
Change-Id: I7ca996618e41b93f488cb5c4de82000bb36e0dd3
Remove 'insertSelectIsSafe' option, unused.
Remove 'topologicalPrimaryConnRef' option, no longer used as of two
months ago with I41a57247503 (8c9398f7f9).
Remove unneeded DatabaseSqlite::getTopologyBasedServerId
implementation which can inherit null instead of overriding with string
of "0". Only caller is SqlBagOStuff::makeTimestampedModificationToken
which can be used as MainStash DB, where its important that a given
server always has the same unique name within a set of db hosts that
may replicate to each other. By inheriting null as topology server ID,
it SqlBagOStuff will use IDatabase::getServerName instead. That in turn
uses the 'host' connection parameter, which defaults to null in
DatabaseFactory, and then falls back to the string "unknown" which is
as good as "0" for this purpose.
Bug: T299691
Change-Id: Iceb65c28cdd3c4a89b3c8b34c3f95d3285718ec0
This method is unused and the naming is not very clear in terms of what
it returns (server name vs config map). Removing it reduces externally
exposed complexity.
Optimize DBConnRef::getServerName() when the server index is known.
Use this to eliminate the "topologicalMaster" parameter from Database.
Rename internal fields and paramters in Database to use "primary"
instead of "master", for consistency.
Add some additional clarifying comments.
Bug: T299691
Change-Id: I98515fa02a58a4c72a06f1ff283b249b1617c886
This was written for the scenerio where we would support writes
on a primary DC request, immediately followed by a secondary DC
request, and we'd then first use the CP cookie to wait-for Redis
replication to communicate the MySQL position data, and then wait-for
a MySQL replica to have caught up to that point.
In the revised 2020 plan for Multi-DC (T254634), CP data is kept
dc-local only, and we use a sticky-dc cookie and other traffic
routing to remain in the primary DC for certain scenarios.
CP remains as playing a rule to ensure consistency within the primary
DC between the write request and the next few seconds of GET requests
to ensure a replica is picked or waited-for to have caught up with
prior writes in the same primary DC.
One test needed to be updated as this commit exposed a happy accident
where previously one key ('writeIndex') was unset in the mocked
array, which was tolerated with the null-ish operator. Now that we
use is_array only and then access the key, this undefined offset
got exposed. Fix by removing the hardcoded logic and instead
use $cp->key and $cp-mergePositions(), which also helps gain more
confidence in the test by relying on their side-effects.
Bug: T314434
Change-Id: I712e5aeb6712f4038011c3c48799e859aa40009d
This introduces $wgLBFactoryConf['configCallback'] which can be set to a
function that returns updates to be applied to $wgLBFactoryConf. The new
method LBFactory::autoreConfigure() can be called to check the callabck
and, if the config changed, reconfigure all existing LoadBalancers.
Reconfiguring the LoadBalancers causes all open connections to be
invalidated; however, any DBConnRef instances will remain valid and will
acquire a fresh connection from the LoadBalancer automatically when
appropriate.
As a proof of concept, this patch adds support for config reloding
into WikiExporter.
Bug: T298485
Change-Id: I6c3ffde62f6e038730736abe980befd90ec43e1a
createMock() does the same, but is much easier to read.
A small difference is that some of the replacements made in this
patch didn't use disableOriginalConstructor() before. In case this
was relevant we should see the respective test fail. If not we can
save some CPU cycles and skip these constructors.
Change-Id: Ib98fb06e0fe753b7a53cb087a47e1159515a8ad5
Use a single callback for both mocks. This is needed because using
PHP 8.1 results in:
> 1) LBFactoryTest::testChronologyProtector
> Implicit conversion from float 1657063075.187724
> to int loses precision
>
> tests/phpunit/includes/db/LBFactoryTest.php:313
> includes/libs/rdbms/ChronologyProtector.php:290
Bug: T312183
Change-Id: I2764cf9b81b08832c904bf7245bd54ca0f0eddad
This would simplify the code for its users a lot.
Bug: T255493
Depends-On: I6ab4440a13f4682dd3a6e9cfc6c629222fbeab8a
Change-Id: I6e7544763bde56fc1e19de0358b71ba984a978ca
Callback style iteration made sense before generators existed, but
generators make for simpler code. The "call method" variants made
sense before closures existed but defeat static analysis.
So, in LBFactory:
* Add ILBFactory::getAllLBs()
* Deprecate ILBFactory::forEachLB()
* Remove LBFactory::forEachLBCallMethod(), was protected.
* Add LBFactory::getLBsForOwner(), which is protected and has the
internal interface in @return. Adding a new abstract method breaks
Wikibase tests despite LBFactory not being stable to extend.
* Migrate callers. Generators allow you to return/break from the middle
of the loop, which implies a little rearrangement for some callers.
In LoadBalancer, connections supposedly of type IDatabase were
routinely type-hinted as Database in closure parameters so that methods
could be called that were not in the interface. So it's convenient to
get rid of public iteration methods entirely in favour of private
methods returning Database[].
* Hard-deprecate ILoadBalancerForOwner::forEachOpenConnection()
and replace it with a private generator method since nothing called
it externally except for core tests.
* Hard-deprecate ILoadBalancerForOwner::forEachOpenPrimaryConnection()
and replace it with a private generator. DeferredUpdates needed it for
iterating over IDatabase::explicitTrxActive(), so add
ILoadBalancer::explicitTrxActive() as a replacement.
* Replace private method LoadBalancer::forEachOpenReplicaConnection()
with a generator.
Depends-On: If0b382231e27d6d1197fb7b6aef6ab50335df4e5
Change-Id: I64514e77b9bfe737be5b12e1d3c9c49976bb522f
Follows-up I361fde0de7f4406bce6ed075ed397effa5be3359.
Per T253461, not mass-changing source code, but the use of the native
error silencing operator (@) is especially useful in tests because:
1. It requires any/all statements to be explicitly marked. The
suppressWarnings/restoreWarnings sections encourage developers to
be "lazy" and thus encapsulate more than needed if there are multiple
ones near each other, which would ignore potentially important
warnings in a test case, which is generally exactly the time when
it is really useful to get warnings etc.
2. It avoids leaking state, for example in LBFactoryTest the
assertFalse call would throw a PHPUnit assertion error (not meant
to be caught by the local catch), and thus won't reach
AtEase::restoreWarnings. This then causes later code to end up
in a mismatching state and creates a confusing error_reporting
state.
See .phpcs.xml, where the at operator is allowed for all test code.
Change-Id: I68d1725d685e0a7586468bc9de6dc29ceea31b8a
The global function wfWikiID() is deprecated since 1.35 and it's usages
should be replaced with WikiMap::getCurrentWikiId().
Bug: T298059
Change-Id: I22d96b7aec17323d15a9bc401d4511ad2ee14165
This is the default anyway when using ->with(). The test code
becomes so much more readable without this, I would like to
argue. Let it just say "with these values".
Because of the way I split my changes into multiple patches
there are a few other changes in this patch I could not split,
e.g. removing unnecessary ->any(). This is the default anyway
and doesn't make the test more specific.
Change-Id: I34990799fa9258ba8dc64c7e78ec43f7903b7681
Remove WRITE_SYNC flag from ChronologyProtector since the current
plan is to simply use a datacenter-local storage cluster.
Move the touched timestamps into the same stash key that holds the
replication positions. Update the ChronologyProtector::getTouched()
comments.
Also:
* Use $wgMainCacheType as a $wgChronologyProtectorStash fallback
since the main stash will be 'db-replicated' for most sites.
* Remove HashBagOStuff default for position store since that can
result in timeouts waiting on a write position index to appear
since the data does not actually persist accress requests.
* Rename ChronologyProtector::saveSessionReplicationPosition()
since it does not actually save replication positions to storage.
* Make ChronologyProtector::getTouched() check the "enabled" field.
* Allow mocking the current time in ChronologyProtector.
* Mark some internal methods with @internal.
* Migrate various comments from $wgMainStash to BagOStuff.
* Update some other ObjectCache related comments.
Bug: T254634
Change-Id: I0456f5d40a558122a1b50baf4ab400c5cf0b623d
The name change happened some time ago, and I think its
about time to start using the name name!
(Done with a find and replace)
My personal motivation for doing this is that I have started
trying out vscode as an IDE for mediawiki development, and
right now it doesn't appear to handle php aliases very well
or at all.
Change-Id: I412235d91ae26e4c1c6a62e0dbb7e7cf3c5ed4a6
There is native support for all of this now in PHP, thanks to changes
and additions that have been made in later versions. There should be no
need any more to ever use call_user_func() or call_user_func_array().
Reviewing this should be fairly easy: Because this patch touches
exclusivly tests, but no production code, there is no such thing as
"insufficent test coverage". As long as CI goes green, this should be
fine.
Change-Id: Ib9690103687734bb5a85d3dab0e5642a07087bbc
The one caller of LBFactory::getChronologyProtectorTouched() was calling
it with a domain ID instead of a master server name. Using the master
server name to identify replication position makes sense for
ChronologyProtector, since the replication position may be reset when
the master changes, but it is an odd convention for LBFactory. So:
* Rename all $dbName variables in ChronologyProtector to $masterName,
for clarity.
* Interpret the first parameter to
ILBFactory::getChronologyProtectorTouched() as a database domain, to
make its only existing caller work.
* Change the first parameter to ChronologyProtector::getTouched() from a
string to a strongly typed ILoadBalancer, by analogy with
applySessionReplicationPosition(). This removes the master name concept
from the public interface.
* Mark ChronologyProtector @internal. The accessor in LBFactory is
protected, so extensions can't use it anyway. This is just to clarify
why I think changing the parameter to getTouched() without b/c is OK.
* Add a simple test which mostly just checks that ChronologyProtector gets
called with the correct parameters. It's an
LBFactory/ChronologyProtector integration test.
Change-Id: I3b4832b5a4d7410e94b9c51577b30b31d49bc63d
Done with `composer fix` and suppressing the rest (i.e. sniffs for
global variables, which for core should be suppressed anyway).
Additionally, add `-p` to `phpcbf`, as otherwise it just seems stuck.
Change-Id: Ide8d6cdd083655891b6d654e78440fbda81ab2bc
This allows full configuration of the LoadMonitor, not just setting
the class name like loadMonitorClass allows. Deprecate the later.
Also fix obsolete LoadBalancer constructor documentation and
standardize the style of LBFactoryMulti::__construct() comments.
Change-Id: Icfdc0d6bbf227dce56e65ad679b93ce3f604e9f7
Add new ILBFactory::setDomainAliases() method for injection database
domain aliases and call it in MWLBFactory::setDomainAliases().
Also:
* Remove overkill "last db/section" caching in LBFactoryMulti
* Clean up some LBFactoryMulti code comments
* Split out separate MWLBFactoryTest test file
Change-Id: If180a58c61178969ca7587c4a06b8786574c7254
