Guard against exceeding memcache size limits (Bug 781993) #26

Merged
merged 2 commits into from Jan 13, 2014

Conversation

Projects
None yet
2 participants
Owner

rfk commented Dec 5, 2012

This patch for mcclient.py adds explicit encode/decode methods for both keys and values. They provide a convenient hook to implement size-limit checking as described in https://bugzilla.mozilla.org/show_bug.cgi?id=781993

@rafrombrc rafrombrc commented on an outdated diff Dec 6, 2012

mozsvc/storage/mcclient.py
+ key = self.key_prefix + key
+ if len(key) > self.max_key_size:
+ raise ValueError("value too long")
+ return key
+
+ def _decode_key(self, key):
+ assert key.startswith(self.key_prefix)
+ return key[len(self.key_prefix):]
+
+ def _encode_value(self, value):
+ value = json.dumps(value)
+ if len(value) > self.max_value_size:
+ raise ValueError("value too long")
+ return value, 0
+
+ def _decode_value(self, value, flag):
@rafrombrc

rafrombrc Dec 6, 2012

Member

What's the purpose of the flag argument here if it's unused?

Member

rafrombrc commented Dec 6, 2012

This mostly looks good, I'm just a little confused about the introduction of the flags arguments, the usage and intent seems a little unclear and partially baked. Did a memcache API change? Is the flags stuff a bit of a placeholder for add'l features coming soon?

Owner

rfk commented Dec 7, 2012

That's me with my future-proofing hat on. "flag" is a standard memcache thing that lets you flag what format the data is in. We don't use it here, but some other python libs use it to differentiate between a raw string (flag=0), a pickled object (flag=1), etc.

I was thinking you could override these methods to customize the serialization, in which case you might need control over flag.

Member

rafrombrc commented Dec 10, 2012

Okay, that makes sense. I'd say to maybe add a comment to that effect in _decode_value. Also, although less important, you might consider changing the names of your local variables so there's some consistency... right now it's flags in most places but flag in _decode_value. There might be a good reason for this, though, so if there is it's fine, maybe a comment would be useful.

Once this is done you can go ahead and merge. :)

@rfk rfk added a commit that referenced this pull request Jan 13, 2014

@rfk rfk Merge pull request #26 from mozilla-services/rfk/memcached-size-limits
Guard against exceeding memcache size limits (Bug 781993)
ce6cf71

@rfk rfk merged commit ce6cf71 into master Jan 13, 2014

Owner

rfk commented Jan 13, 2014

tada!

rfk deleted the rfk/memcached-size-limits branch Jan 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment