Problem/Motivation
The order of multiple keys from \Memcache::get()
when called with an array isn't guaranteed to match the requested key order. This can be difficult to reproduce locally, but I have reproduced in an environment with multiple webheads (each running their own memcache that are then chained together).
This issue produces things like random ordering of nodes on the node/add
route, and may have even more, yet to be discovered, serious consequences.
Proposed resolution
For \Memcached::getMulti()
use Memcached::GET_PRESERVE_ORDER
.
For DrupalMemcache::getMulti()
, add an array merge with the original keys to ensure order is preserved.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#10 | 2562833-9.patch | 1.11 KB | damiankloip |
#7 | 2562833-07.patch | 1.03 KB | jhedstrom |
Comments
Comment #2
jhedstromAnd the patch.
Comment #3
damiankloip CreditAttribution: damiankloip commentedThis makes sense, I am all for consistency :) Just a couple of things:
Instead of doing this why don't we just iterate over the cid lookup array and create the $cid_results return array from that order? We have to iterate anyway..
This is not really a NULL, maybe we just call this $cas or something?
Comment #4
jhedstromThis addresses the latter part of #3. I started to look at iterating, but we aren't already iterating over the full keys, so I'm not sure if that approach would be any better than the
array_merge
approach.Comment #5
damiankloip CreditAttribution: damiankloip commentedjhedstrom, this looks like it also contains the original patch from #2467005: bin gets ignored in cache key unless each bin is declared in settings. Fancy removing that, then we can get this in.
Comment #6
damiankloip CreditAttribution: damiankloip commented1 char over the limit :)
Comment #7
jhedstromOops. Here's the patch from #4 without the other fix in it. Also addressed #6.
Comment #8
damiankloip CreditAttribution: damiankloip commentedThat looks good, but I just had a thought. By this point, the $cid_results has the $cids that were passed into the call rather than the memcache keys. It's quite possible that a cid could just be a numeric value, like an entity ID? If so, array_merge would append instead of replace the values?
We don't have this in core itself, but a custom cache table could easily do that.
Comment #9
dawehnerCan't we leverage array_intersect_key or so to sort the result by the CIDs?
Comment #10
damiankloip CreditAttribution: damiankloip commentedLike this instead maybe.
EDIT: Doh, forgot the comment from your previous patch!
Comment #11
damiankloip CreditAttribution: damiankloip commented@dawehner sorry missed your comment there! That is basically what this patch does now. We were thinking the same I guess.
Comment #13
damiankloip CreditAttribution: damiankloip commented