https://bugzilla.wikimedia.org/show_bug.cgi?id=59623
--- Comment #3 from Krinkle ---
(In reply to comment #2)
> The underlying Redis library's get() is documented as returning stuff as a
> String type, and PECL memcached's get() is documented as returning mixed
> types.
>
> Modification of RedisBagOStuff's serialize and unserialize to make it more
> like PECL memcached-like would break the Redis INCR/DECR commands.
>
Can you elaborate on this? Afaik memcached has incr/descr like commands as
well. However afaik this shouldn't be an issue. It is up to the BagOStuff
subclass to ensure stuff roundtrips properly.
In case of Memcached, which supports scalars natively, it does PHP
serialise/unserialise for anything else, thus turning it into a string and
ensuring proper roundtrip. And integers presumably stay integers within
Memcached as well.
If Redis doesn't support integers separate from strings, would it be sensible
to abstract this limitation in our BagOStuff subclass and serialise them in
such a way that we get back out what we put in?
I take it serialising integers in a string (e.g. "int/12345" or using php
serialise() format) is problematic as one can then no longer do INCR/DECR on
it. But I'm confused though as to how Redis does this internally. Surely it has
to distinguish between numbers and strings in some way in order to support
INCR/DECR at all?
Would it perhaps be sensible to do it the other way around (have the Redis
BagOStuff get() method convert the string to integer before returning if it
looks like something that should be an integer). Might als cause side effects,
as a string might come out as an integer that way, but then again, the opposite
is causing side effects as well (as proven by the existence of this bug).
I'm fine with work arounds, but the reason I'm somewhat leery of implementing
this workaround is because it seems the wrong place to deal with this.
The code I wrote there assuming an integer comes out when an integer is put in,
I'm happy to be proven wrong, but I wasn't aware of that being unreliable. From
my perspective it is our RedisBagOStuff implementation that is inconsistent
with the status quo (I understand now why RedisBagOStuff does this, but that
doesn't make it right to break backwards compatibility).
Basically, if we accept this work-around, we should start actively discouraging
usage of actual integers in cache storage (in that, we shouldn't assume they
come out as integers, regardless of which cache storage). So perhaps we should
then modify the MemcachedBagOStuff to convert integers to strings in get()?
> Given that we know getDefinitionMtime needs to return an integer type, can we
> move forward with this? If not, what do you propose?
Short of changing RedisBagOStuff::get just for this, I'd recommend changing the
expression assigned to $data from $cache->get( $key ); to (int) $cache->get(
$key ) in:
https://gerrit.wikimedia.org/r/#/c/103407/3/includes/resourceloader/ResourceLoaderModule.php
And then have only one if branch, not two. Having two if-branches there is
highly confusing in my opinion and bound to get messed up in future maintenance
as there is no obvious reason for the $data type to be variable. This should be
abstracted by the cache storage, just like we do for database abstractions.
That sometimes means we can't make use of all the features a storage system has
to offer, but I guess that's worth it for the benefit of consistency and
predictability (in this case we'd give up getting integers from Memcached, we
could still keep them as integers in Memcached internally and probably have to
for INCR/DECR to work there, but we'd patch our get() to always return a
string, like Redis does).
Anyway, let's handle the cache storage system inconsistency in a separate bug.
> I'm fine unbasing the dependent change for the URL given that we know in
> production things will continue working, but I also want to future proof this
> in case the memory cache provider changes.
I'm not sure what you mean here.
--
You are receiving this mail because:
You are the assignee for the bug.
You are on the CC list for the bug.
___
Wikibugs-l mailing list
Wikibugs-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
