WIP - Fix key order dependent multi_get errors #3
Conversation
* not yet sure if this works...but I'm still curious and kept this just in case it points toward being useful * include generated code running more recent swig...which is a bit noisy :-)
* I can't find this documented, but I thought that we were always returning errors even if some keys could be resolved...turns out that is key-order dependent currently. The "fix" was to ignore the `memcached_mget` return value and re-raise it if we nothing useful was returned upon calling `memcached_fetch_rvalue`. The only docs https://awesomized.github.io/libmemcached/libmemcached/memcached_get.html I could find indicate > To retrieve data after a successful execution of memcached_mget(), you > will need to call memcached_fetch_result(). You should continue to call > this function until it returns a NULL (i.e. no more values). which feels a little underspecified (we're also like infinity versions behind :-)?
| if (result == NULL || *error != MEMCACHED_SUCCESS) { | ||
| if (result == NULL) { |
There was a problem hiding this comment.
This change was intended to test out whether we can get additional info out of the fetch calls. I haven't yet seen any indication that this is helpful or harmful, the test suite passes, but this shouldn't be material to the change that allows us to consistently raise errors. And dropping this change would clear a lot of the diff because it require re-SWIG-ing.
| value, key, flags, ret = Lib.memcached_fetch_rvalue(@struct) | ||
| check_return_code(mget_ret, keys) if ret == Lib::MEMCACHED_END || key == "" # raise original return code if nothing can be fetched |
There was a problem hiding this comment.
As mentioned in the PR, the docs (that are quite a few versions away) say to call fetch until it returns NULL assuming success. We do appear to get more consistent behavior when we try to read the result even when the memcached_mget call returns an error code?
| # query both servers, and ensure order doesn't matter | ||
| get_res1 = cache.get(["foo", "bar"]) | ||
| assert_equal({"bar" => "bar"}, get_res1) | ||
|
|
||
| get_res2 = cache.get(["bar", "foo"]) | ||
| assert_equal({"bar" => "bar"}, get_res2) |
There was a problem hiding this comment.
I want to test this under more failure modes, but preliminary data suggests that it's possible that if the last key raises a network error, memcached_mget will return that error even though it's possible to loop through fetching values for the servers that were able to respond. That's my current hypothesis, at least!
I wrote a test intending to document that we raise in cases when a subset of keys can be resolved.
In writing that test I discovered the behavior is potentially key-order dependent.
I'm going to do some more work to see in which ways these manifest (do timeouts behave the same?)...but in general the problem is that has a potentially outsized impact.
In the "success" case where we resolve a subset of keys, its currenty unclear to the caller that retries might be in order.
In the failure case where we raise even though a subset of keys can be resolved, its reasonable for the caller to retry but they'll necessarily have to retry the entire set, which is wasteful.