This method was deprecated in 1.42 and now can be dropped from
MW core.
In addition, some cosmetic changes in this patch:
* Type-hint `isSegmented()` to return bool.
Bug: T344521
Change-Id: Idace008e9a961953041bd21b499bfec3f8226142
Since the method for constructing a serialized value container via
`newUnified()` has been removed in Id5179be032d45732db659d506d2781,
its corresponding `isUnified()` method is now obsolete and this
patch deprecates it to be removed in 1.43.
Bug: T344521
Change-Id: I5a90028c2a596dfec9a1df306cc9d0ab90dfedd6
This patch deletes the `::newUnified()` method from this class, it's
already hard deprecated and due for removal.
Bug: T344521
Change-Id: Id5179be032d45732db659d506d2781752f03f8be
Fixed SkinModuleTest::provideGetFeatureFilePathsOrder as nesting of
arrays for parameters is wrong
Change-Id: I9875008adf62d284c48662ebfbd245d72e5be064
This patch migrates BagOStuff metrics capture from StatsdD to
StatsLib. `updateOpStats()` definition significantly changed with
this migration thereby affecting the changes in the test case.
What we tried to do in the test is to assert that the metrics
match the StatsLib equivalent ones which is the new format we're
using and that the values corresponding to the samples are also
in the range of values written to the cache.
Notes
=====
* And the cache key also uses underscore (_) instead of (.) because
of StatsLib as well.
* The most important thing which is tested is that the keys inserted
into the cache are all there and correspond to their values. The way
we do this is observe the metrics stream after `->flush()`.
Bug: T356062
Change-Id: Ia6c14746de5bddeaca7917c76f1c9d1e100ae2b2
* Switch out raw Exceptions, mostly for InvalidArgumentExceptions.
* Fake exceptions triggered to give Monolog a backtrace are for
some reason "traditionally" RuntimeExceptions, instead, so we
continue to use that pattern in remaining locations.
* Just entirely give up on PostgresResultWrapper's resource vs. object mess.
* Drop now-unneeded false positive hits.
Change-Id: Id183ab60994cd9c6dc80401d4ce4de0ddf2b3da0
Updating name & email addresses for Brooke Vibber.
Re-ran updateCredits.php as well so there are some new entries in
there as well.
There are a couple of files in resources/libs that will have to
be changed upstream to keep tests happy, I will do patches
later. :D
Change-Id: I2f2e75d3fa42e8cf6de19a8fbb615bac28efcd54
Update cases where one of the IConnectionProvider methods is called
immediately.
This doesn't really change anything, but I hope it helps promote
getConnectionProvider() as the common way to do this.
Follow-up to 8604c384f6.
Change-Id: Id0e7d02bab0c570343c2b1f03c70b44ee39db112
The MainStash use case is already covered by the use of CACHE_DB
(SqlBagOStuff) and at this point, we no longer have any uses of
ReplicatedBagOStuff. So, it can be deprecated and removed.
Bug: T352481
Change-Id: Id691eafd60040792ae8ab767ee6013c1681461c1
For readability. Allowed since PHP 7.4.
I searched for integer literals of 6 or more digits, and also changed
some nearby smaller numbers for consistency.
Bug: T353205
Change-Id: I8518e04889ba8fd52e0f9476a74f8e3e1454b678
Introduce a new interface Wikimedia/Http/TelemetryHeadersInterface
that provides telemetry information that could be attached to
HTTP Requests. MultiHttpClient is expecting `telemetry` option
of TelemetryHeadersInterface type.
The MediaWiki/Http/Telemetry implements the interface, therefore
ObjectCache can inject it to RESTBagOStuff, that further injects
it to MultiHttpClient.
Bug: T344926
Change-Id: I59a3f1048c403fe2e4ef0c74353dfe74ff9ca893
Introduced in I0667a02612526d8ddfd91d5de48b6faa78bd1ab5 (in 2019)
and used for consistency by default in BagOStuff::set(), even for the
99% of values that don't need segmentation.
The method was removed from use in I830c78a50efd1ba83fbe2aa39c1, due
to the overhead of SerializedValueContainer being undesirable, so there
is no longer a use case for it. If a value doesn't need segmentation,
it shouldn't be wrapped in SerializedValueContainer.
See https://codesearch.wmcloud.org/search/?q=SerializedValueContainer%3A%3AnewUnified&files=&excludeFiles=&repos=
Bug: T344521
Change-Id: Id1c283201cd42c4eabac8ef4d949329959016b18
When getWithSetCallback() is called with check keys, if the keys are
missing, a check key is inserted with the current time, as if
touchCheckKey() were called. This causes cache misses for
HOLDOFF_TTL = 11 seconds. This seems unnecessary since in the case of
an empty cache, there is no expectation of replication delay.
However, it's reasonable for it to be a cache miss when the check key is
missing, and a cache hit subsequently, so we do need to add a purge
value.
So, in getWithSetCallback(), set the holdoff to zero when inserting a
purge value.
Also, use a holdoff of zero when initialising a missing touch key in
getCheckKeyTime().
Bug: T344191
Change-Id: Ib3ae4b963816e5b090e87e4cb93624afefbf8058
Introduced in I4060b19583cdfd9fa36c91d7014441eeef4b3609 and removed
in I62ec1c269ffb87eca03003b3f3c5336ee562a7b3 but the parameter was
not cleaned up.
This patch removes the parameter from the method and callers including
other methods that passed this in.
NOTE: These are all private methods so we can go ahead and remove this
paramter without deprecation.
Change-Id: Ifa76334e671ba8a8577be886bd407a6ac8038fa6
Interface was deprecated since 1.35 in favor of ExpirationAwareness
or StorageAwareness interfaces. No longer used anywhere, see dependent
patches.
Depends-On: Ida557b3180eb5e7ebae46968142b4f154f26ffbc
Depends-On: I3d6fbf535560655472ade27c37b0e42b3e11a535
Depends-On: I2b0a669d41d9e6a8a859cba314c0e9e4c0ef40d7
Depends-On: I776040c2c8f61e25ae986e93bb1975fdd8bf9dd5
Change-Id: I9c5ffa9f51aec6356e3e27458fd098a37cd754ad
== Background
Most of this was introduced in commit 5c335f9d77 (I1eb897c2cea3f5b7).
The original motivation was:
* Ensure wrappers like MultiWriteBagOStuff naturally do the right
thing. In practice, makeKey() results are interchangeable, with
the most contrained one (Memcached) also generally used as the first
tier. However, this is not intuitive and may change in the future.
To make it more intuitive, the default implemention became known
as "generic", with proxyCall() responsible for decoding these,
and then re-encoding them with makeKey() from the respective
underlying BagOStuff. This meant that MultiWriteBag would no longer
use the result of the Memcached-formatted cache key and pass it
to SqlBagOStuff.
* Allow extraction of the key group from a given key cache,
for use in statistics.
Both motivations remains valid and addressed after this refactor.
== Change
* Remove boilerplate and indirection around makeKey from a dozen
classes. E.g. copy-paste stubs for makeKey, makeKeyInternal, and
convertGenericKey.
Instead, let BagOStuff::makeKey and ::makeKeyInternal hold the
defaults. I believe this makes the logic easier to find, understand,
and refer to.
The three non-default implementations (Memcached, WinCache, Sql)
now naturally reflect what they are in terms of business logic,
they are a method override.
Introduce a single boolean requireConvertGenericKey() to let the
three non-default implementations signal their need to convert
keys before use.
* Further improve internal consistently of BagOStuff::makeKeyInternal.
The logic of genericKeyFromComponents() was moved up into
BagOStuff::makeKeyInternal. As a result of caling this directly
from BagOStuff::makeKey(), this code now sees $keyspace and $components
as separate arguments. To keep the behaviour the same, we would
have to either unshift $keyspace into $components, or duplicate
the strtr() call to escape it.
Instead, excempt keyspace from escaping. This matches how the most
commonly used BagOStuff implementations (MemcachedBag, and SqlBag)
already worked for 10+ years, thus this does not introduce any new
responsibility on callers. In particular, keyspace (not key group)
is set by MediaWiki core in service wiring to the wiki ID, and so
is not the concern of individual callers anyway.
* Docs: Explain in proxyCall() why this indirection and complexity
exists. It lets wrapping classes decode and re-encode keys.
* Docs: Explain the cross-wiki and local-wiki semantics of makeKey
and makeKeyGlobal, and centralise this and other important docs
about this method in the place with the most eye balls where it is
most likely seen and discovered, namely BagOStuff::makeKey.
Remove partial docs from other places in favour of references to this one.
Previously, there was no particular reason to follow `@see IStoreKeyEncoder`
much less to know that it holds critical that communicate the
responsibility to limit the key group to 48 chars.
* Docs: Consistently refer to the first component as the "key group",
thus unifying what was known as "key class", "collection",
"key collection name", or "collection name".
The term "key group" seems to be what is used by developers in
conversations for this concept, matching WMF on-boarding docs and
WMF's Grafana dashboard for WANObjectCache.
Change-Id: I6b3167cac824d8bd8773bc66c386f41e4d380021
This was introduced for use by WANObjectCache, however, this never
worked in practice.
The feature only influences determineKeyPrefixForStats(), which in
turn is only called by MediumSpecificBagOStuff::updateOpStats, which
in turn is exclusively called within BagOStuff subclasses that aren't
used with WANObjectCache (RESTBag, RedisBag, SqlBag).
The next commit simplifies BagOStuff::makeKey, which can make a bigger
impact if this feature doesn't need to be supported.
During drafting of the next commit, I found that the feature doesn't
appear to work well in practice, because getCollectionFromSisterKey
did not account for mcrouter-style route prefixes, as inserted by
makeSisterKey().
Change-Id: Iecea959c88abbd7a0a17b92c1c4d854602748280
When enabling $wgResourceLoaderUseObjectCacheForDeps in I79299f80fe06,
I get the following warning:
> PHP Warning: Invalid argument supplied for foreach()
> > BagOStuff.php(772): MWExceptionHandler::handleError()
> > ReplicatedBagOStuff.php(218): BagOStuff->proxyCall()
> > KeyValueDependencyStore.php(81): ReplicatedBagOStuff->setMulti(array, …)
> > ResourceLoader.php(547): KeyValueDependencyStore->storeMulti()
> > …
This is happening because setMulti is configured as RES_KEYMAP,
which sets the expectation of an array return value, when actually
BagOStuff::setMulti returns bool.
There is similar code in MultiWriteBagOStuff, which already used
RES_NONKEY correctly.
Change-Id: Ifc9db0758f0e150862d163eb7a4df03042f54e83
Use str_starts_with, str_ends_with or string offset where appropriate.
This fixes a bug in MimeAnalyzer where the "UTF-16LE" header could not
be identified because of wrong constant. This is the exact type of bug
that the new functions can avoid.
Change-Id: I9f30881e7e895f011db29cf5dcbe43bc4f341062
* Combine the scripts.
* Use luaEval() to reduce the network overhead.
* Remove "r" prefix on local variables
Change-Id: I7c3097e18dd8570023d9a711327e139e395dfbbb
* Unnecessary regex modifier. I agree with this inspection which flags
/s modifiers on regexes that don't use a dot.
* Property declared dynamically.
* Unused local variable. But it's acceptable for an unused local
variable to take the return value of a method under test, when it is
being tested for its side-effects. And it's acceptable for an unused
local variable to document unused list expansion elements, or the
nature of array keys in a foreach.
Change-Id: I067b5b45dd1138c00e7269b66d3d1385f202fe7f
* Illegal string offset and invalid argument supplied to foreach, due to incorrect type information
* Array internal pointer reset is unnecessary
* $hookData unused since MW 1.35 due to incomplete revert
* array_push() with single element
* Unnecessary sprintf()
* for loop can be replaced with str_repeat()
* preg_replace() can be replaced with rtrim()
* array_values() call is redundant
* Unnecessary cast to string
* Unnecessary ternary. Often the result relies on short-circuit evaluation, but I find it more readable nonetheless.
Change-Id: I4c45bdb59b51b243fa96286bec8b58deb097d707
Previously, a bug resulted in a connection per key for getMulti()
and similar methods. RedisConnRef instances are treated as if they
belong to different callers, so RedisConnectionPool::getConnection
cannot be used in naive loops to build (key => RedisConnRef) arrays.
Create a RedisBagOStuff::getConnectionsForKeys() method to properly
get connection handles for multiple keys and reduce code duplication.
Bug: T332371
Change-Id: Id58b9ac61c59277f7cd8f17821c1905b930b02cb
Return the new value as per the base class. Also, avoid use
the "setex" command when the TTL is zero (which is invalid).
This fixes a failing test in RedisBagOStuffIntegrationTest.
Follow-up to 79ea356f1b.
Change-Id: Ia33bed959928788252fdf7bf37d56fa9e5946765
This value was unused and rarely set. LOW_TTL should be sufficient to
avoid stampedes without needing to use the generation time cost.
Change-Id: I62ec1c269ffb87eca03003b3f3c5336ee562a7b3
Make it use a power function and increase LOW_TTL from 30 to 60 so
that higher regeneration times (e.g. 1+ seconds) are more resistant
to regeneration stampedes.
Also fix code that was using LOW_TTL instead of TTL_LAGGED.
Bug: T331914
Change-Id: I2c0b35fbbce6b5a692b6d7885a2a921d7ffdd031
