https://bugzilla.wikimedia.org/show_bug.cgi?id=59623
--- Comment #3 from Krinkle <krinklem...@gmail.com> --- (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
