linsomniac / python-memcached

A python memcached client library.
460 stars 201 forks source link

Can't get, or set 'NFL::CAR_TB' #21

Closed seaders closed 10 years ago

seaders commented 10 years ago

After checking a whole loada things, I've found that for some keys, python-memcached just won't get, or set them on my machine (homebrew'd 1.4.15, pip installed python-memcached 1.53 on Mac OSX 10.9). My first issue was a shot in the dark, not having a clear idea as to what was going on, but after more digging I now definitely now.

It all hinges around def _get_server(self, key):

If we add two debug printout lines,

def _get_server(self, key):
    if isinstance(key, tuple):
        serverhash, key = key
    else:
        serverhash = serverHashFunction(key)

    for i in range(Client._SERVER_RETRIES):
        server = self.buckets[serverhash % len(self.buckets)]
        if server.connect():
            #print "(using server %s)" % server,
            print 'got server {} for {}'.format(serverhash % len(self.buckets),
                                                key)
            return server, key
        print 'server {} failed for {}'.format(serverhash % len(self.buckets),
                                            key)
        serverhash = serverHashFunction(str(serverhash) + str(i))
    return None, None

Trying to get or set the key 'NFL::CAR_TB',

import memcache
mc_cl = memcache.Client('127.0.0.1')
mc_cl.set('NFL::CAR_TB', 1)
mc_cl.get('NFL::CAR_TB')
mc_cl.set('NFL::CAR_UB', 1)
mc_cl.get('NFL::CAR_UB')

results, on my machine with,

server 7 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 8 (inet:1:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 3 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 8 (inet:1:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 0 (inet:1:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 7 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 7 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 5 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 1 (inet:2:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 5 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 7 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 8 (inet:1:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 3 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 8 (inet:1:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 0 (inet:1:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 7 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 7 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 5 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 1 (inet:2:11211 (dead until 1384820703)) failed for NFL::CAR_TB
server 5 (inet:.:11211 (dead until 1384820703)) failed for NFL::CAR_TB
got server 6 for NFL::CAR_UB
got server 6 for NFL::CAR_UB

If I alter _get_server to

def _get_server(self, key):
    choices = range(len(self.buckets) - 1)
    random.shuffle(choices)

    if isinstance(key, tuple):
        choice, key = key
    else:
        choice = choices.pop()

    for _ in range(Client._SERVER_RETRIES):
        server = self.buckets[choice]
        if server.connect():
            #print "(using server %s)" % server,
            print 'got server {} for {}'.format(choice,
                                                key)
            return server, key
        print 'server {} ({}) failed for {}'.format(choice, server, key)
        choice = choices.pop()
    return None, None

Then everything works much better,

got server 4 for NFL::CAR_TB
server 1 (inet:2:11211 (dead until 1384820883)) failed for NFL::CAR_TB
server 7 (inet:.:11211 (dead until 1384820883)) failed for NFL::CAR_TB
server 0 (inet:1:11211 (dead until 1384820883)) failed for NFL::CAR_TB
server 2 (inet:7:11211 (dead until 1384820883)) failed for NFL::CAR_TB
got server 4 for NFL::CAR_TB
server 1 (inet:2:11211 (dead until 1384820883)) failed for NFL::CAR_UB
server 3 (inet:.:11211 (dead until 1384820883)) failed for NFL::CAR_UB
got server 4 for NFL::CAR_UB
server 1 (inet:2:11211 (dead until 1384820883)) failed for NFL::CAR_UB
got server 6 for NFL::CAR_UB

~~ multi_get and multi_set not getting / setting all keys

I was getting very inconsistent results when trying to set multiple keys in my python program,

import memcache

TWO_HOURS = 2 * 60 * 60
mc_cl = memcache.Client('127.0.0.1')
mapping = {...}

mc_cl.flush_all()
ret = mc_cl.set_multi(mapping=mapping, time=TWO_HOURS)
getret = mc_cl.get_multi(mapping.keys())
if len(mapping) != len(getret):
    print 'not set\n\t{}'.format('\n\t'.join([str((k, mapping[k])) for k in
                                              [a for a in mapping.keys()
                                               if a not in getret.keys()]]))

And after analysing the raw memcache output, it seems like not all the keys are being set, and not all the keys are being requested thereafter. All things done and up to date on homebrew and pip, on Mac OSX 10.9.

Memcache output below and reading it states that only 101 keys were attempted to be written and read, whereas there were 228 items.

import re

gamesSet = []
gamesGet = []
with open('memout.log') as f:
    for line in f.read().split('\n'):
        match = re.match('^<21 set ([^ ]+) .*$', line)
        if match is not None:
            gamesSet.append(match.group(1))
            continue
        match = re.match('^<21 get (.*)$', line)
        if match is not None:
            gamesGet += match.group(1).split(' ')
            continue
print len(mapping), mapping
print len(gamesSet), gamesSet
print len(gamesGet), gamesGet

That log is below.

<21 new auto-negotiating client connection
21: Client using the ascii protocol
<21 flush_all
>21 OK
<22 new auto-negotiating client connection
22: Client using the ascii protocol
<22 flush_all
>22 OK
<21 set NCAAF::BUFF_KENTST 1 7200 34
>21 STORED
<21 set NCAAF::ND_USC 1 7200 29
>21 STORED

~~

linsomniac commented 10 years ago

You sure seem to have a lot of dead servers...

I'm pretty sure your problem is that you are calling the Memcache class incorrectly, passing it a string instead of a list of strings. This is why it thinks you have many servers and they are all down and they are failing.

Sean

seaders commented 10 years ago

This is all on my development machine, which, as I said is on Mac OSX 10.9, with memcached 1.4.15 having been installed via homebrew. I've done nothing else to the setup. Since I ran into this problem, I have uninstalled and reinstalled both memcached itself, and python-memcached with pip, and nothing changed.

The only code I'm running to get this behaviour is

import memcache
mc_cl = memcache.Client('127.0.0.1')
mc_cl.set('NFL::CAR_TB', 1)
mc_cl.get('NFL::CAR_TB')
mc_cl.set('NFL::CAR_UB', 1)
mc_cl.get('NFL::CAR_UB')

and it runs the exact same if I do that, or

import memcache
mc_cl = memcache.Client('127.0.0.1')
mc_cl.set_multi({'NFL::CAR_TB': 1, 'NFL::CAR_UB': 1})
mc_cl.get_multi(['NFL::CAR_TB', 'NFL::CAR_UB'])

No matter what, I can't get, or set NFL::CAR_TB.

linsomniac commented 10 years ago

Please re-read my reply and review the examples of use in the memcache code itself, you are incorrectly using the library.

Sean

seaders commented 10 years ago

Aaahh, I'm sorry, I thought you meant during the get/get_multi & set/set_multi calls, not in the initialisation calls of the library. I understand that this problem is down to my misunderstanding and misuse, but should it work as I've described, if you do pass in a string instead of a list of strings? I mean, had the initialisation either failed, or converted my string to a list of strings in init, would that not be a better outcome to happen? In this way, I, or anyone who can even make a typo, get a inconsistent, half-working library rather than one that will throw an error when it's initialised incorrectly, or a fully working one, when it corrects the user's error?

linsomniac commented 10 years ago

In this specific case, it might be worthwhile to check explicitly for a string being passed in and raising an exception. It probably can't just translate it directly to a list, because it is an established library and there may be someone doing something clever with a string-like object that happens to iterate to a list of servers, for example. So for them it would have to fail loudly.

In general, Python's philosophy is that you shouldn't care about what type something is, just whether it supports the actions you want to do on it. Unfortunately, in this case, strings support iteration just like a list does. And worse, the string "1" is a valid memcache server name (because of 0 padding rules in IP addresses).

So, yes, it is unfortunate that this error surprised you and it would be nice if it didn't. But the solution may be more involved than it first appears. I WILL say that in the 5+ years I've been maintaining the library, this has not come up often, if at all.

I can make a couple of suggestions as well. You didn't seem to validate your code with a simpler test-case. When you found that set_multi wasn't working, experimenting with get and set would have fairly quickly shown the bigger problem. Also, you may have too quickly jumped to the conclusion that the problem was in the library instead of your code. Particularly in an established, popular library like memcache, it's at least worth going through your code before assuming that the upstream is broken. Not to say library code out there is flawless, but established code often is good to give the benefit of the doubt.

seaders commented 10 years ago

Oh, I totally understand it was wholly my fault, and unfortunately, I had done other testing, but because the error was so inconsistent, and failing so silently, I first thought I was going crazy, or there was something wrong with my memcache server, or my homebrew install, or my OS install (cos it's a new-ish machine I'm working on).

I didn't assume the library was at fault because it's so well used, and because I've used it 100s of times before, but unfortunately any other time I've used it locally (when working with only 127.0.0.1), I must have simply copied and pasted, but this time, I started my work from scratch and obviously typed in a single string, instead of a list.

And while I totally take your point about this not coming up often, if at all, but due to the inconsistent and silent nature, and when you check the return of a multi_set like

mc_cl.set_multi({'NFL::CAR_UB': 2, 'NFL::CAR_TB': 1}

It returns an empty list, despite not actually setting 'NFL::CAR_TB'. There could be some unfortunate folks like me out there who pass the '127.0.0.1' string, instead of a list to the client, and then even if they do a check to ensure that all keys are being correctly set, they will be told all's ok.

And for me, your point about '1', and also '0' actually, being valid servers highlights the need to ensure that that starting bit is not a string, even the simplest

assert not isinstance(lst, basestring)

Would serve the purpose perfectly.