Make serde pickle version configurable #160

akatrevorjay · 2017-06-03T01:28:39Z

See #156 for more info :)

* origin/master: Reverse backwards compatible change removed apidoc directory from repo add apidoc to .gitignore

akatrevorjay · 2017-06-03T01:31:35Z

My quick hack was reverted. This provides similar functionality while maintaining backwards compatibility with ancient pythons or a mix of py2/py3 and python-memcached.

akatrevorjay · 2017-06-03T01:41:46Z

pymemcache/serde.py

+            except Exception as exc:
+                # This includes exc as a string for troubleshooting as well as providing
+                # a trace.
+                log.exception('Could not depickle value: %s')


I changed this because using the root logger is nearly impossible to troubleshoot, and this should really be exc level.

akatrevorjay · 2017-06-03T03:04:28Z

fin (edit: ish)

akatrevorjay · 2017-06-06T22:04:25Z

How's this look everyone? Any thoughts?

nichochar

This LGTM, I'll leave a couple other people to review before merging though

nichochar · 2017-06-08T16:52:32Z

pymemcache/codecs.py

+        if pickle_version is not None:
+            self.pickle_version = pickle_version
+
+    def from_python(self, key, value):


how about call this function serialize() (here and in metaclass)

I changed it because in my experience serialize and deserialize are not the best names. Using a common anchor it seems to stick with more juniors easier and avoids headache. Still, I can update if you want, but I'd recommend not.

nichochar · 2017-06-08T16:52:41Z

pymemcache/codecs.py

+
+        return value, flags
+
+    def to_python(self, key, value, flags):


Same here with deserialize

nichochar · 2017-06-08T16:53:07Z

pymemcache/codecs.py

+
+    def to_python(self, key, value, flags):
+        """
+        Deserialize a value into a python object.


Thanks for the doc

Nicochar, anytime for you bud.

jogo

Overall I like the idea of not hard coding pickle protocol 0. But in general I am weary of even tiny backwards incompatible changes. As I have experienced the pain of updated dependencies causing things to break.

jogo · 2017-06-09T21:11:12Z

pymemcache/serde.py

+Backwards compatibility with the older serialization api previously
+provided by this module.
+"""
+from . import codecs


I prefer to use absolute imports here.

https://www.python.org/dev/peps/pep-0008/#imports

PEP8 allows relative imports. Is there a reason to not use them and opt for the less featureful alternative?

Let's keep it consistent with the rest of the project, which uses absolute imports. Both positions are defendable but I think there's a strong argument for consistency

done, consistency > opinions

jogo · 2017-06-09T21:11:55Z

pymemcache/codecs.py

@@ -0,0 +1,170 @@
+# Copyright 2012 Pinterest.com


why rename the file?

To provide backwards compatibility

(Without polluting the primary namespace)

It's also a better name.

I think this is fine.

jogo · 2017-06-09T21:24:32Z

pymemcache/serde.py

-    long_type = None
-
-
-FLAG_BYTES = 0


Although I don't think removing these from this file will be a problem it is technically a backwards incompatible change and don't want to break backwards compatibility.

IMO These should have been private to begin with, plus can't you just bump the major? I'll import them into serde.py if you want though.

I don't understand why this is backward incompatible? I think this is OK. Am I missing something @jogo?

I already imported the API methods, and I don't think anyone should really be using constants in an arbitrary location in a library.

jogo · 2017-06-09T21:30:05Z

pymemcache/codecs.py

+    FLAG_PICKLE = 1 << 0
+    FLAG_INTEGER = 1 << 1
+    FLAG_LONG = 1 << 2
+    FLAG_COMPRESSED = 1 << 3  # unused, to main compatibility with python-memcached


./pymemcache/codecs.py:71:81: E501 line too long (83 > 80 characters) FLAG_COMPRESSED = 1 << 3 # unused, to main compatibility with python-memcached

I'll conform to VT100 in an update

jogo · 2017-06-09T21:37:49Z

pymemcache/codecs.py

+            except Exception as exc:
+                # This includes exc as a string for troubleshooting as well as
+                # providing a trace.
+                log.exception('Could not depickle value (len=%d): %s',


unclear why this is better:

import logging try: raise Exception("raised") except Exception as exc: logging.exception("TEST: %s", exc) try: raise Exception("raised") except Exception: logging.exception("TEST")

ERROR:root:TEST: raised Traceback (most recent call last): File "test.py", line 3, in <module> raise Exception("raised") Exception: raised ERROR:root:TEST Traceback (most recent call last): File "test.py", line 9, in <module> raise Exception("raised") Exception: raised

Because from the initial line, you know what the exc was that caused it. This helps when an exception storm hits and you have N of them in your log interspersed.

Logging entries are usually configured with a better format than the default that you used, which contain things such as the process and thread ID.

jogo · 2017-06-09T21:42:00Z

pymemcache/test/test_codecs.py

+import pytest
+import six
+
+try:


We should import this the same way we do elsewhere in this code: from six.moves import cPickle as pickle

While I can do that, it breaks tab completion.

jogo · 2017-06-09T21:43:24Z

pymemcache/test/test_codecs.py

+    def test_pickle_version(self):
+        for pickle_version in range(-1, pickle.HIGHEST_PROTOCOL):
+            self.check(
+                dict(whoa='nelly', humans=u'horrid', answer=42),


I would prefer to use a different key value pair for humans.

Please assist me with what you would like.

Oh, you mean humans aren't horrid? But we are! Lol, I'll update it!

jogo · 2017-06-09T21:55:31Z

pymemcache/codecs.py

+        """
+        Init
+
+        :param int pickle_version: Pickle version to use (from python).


It's not the pickle version but rather the pickle protocol version

https://docs.python.org/3/library/pickle.html#data-stream-format

Thanks, will update.

nichochar

A couple minor changes, but looking good. Thanks for baring with us @akatrevorjay, we really appreciate the work

nichochar · 2017-06-17T03:20:30Z

pymemcache/serde.py

+Backwards compatibility with the older serialization api previously
+provided by this module.
+"""
+from . import codecs


Let's keep it consistent with the rest of the project, which uses absolute imports. Both positions are defendable but I think there's a strong argument for consistency

nichochar · 2017-06-17T03:21:10Z

pymemcache/serde.py

-    long_type = None
-
-
-FLAG_BYTES = 0


I don't understand why this is backward incompatible? I think this is OK. Am I missing something @jogo?

nichochar · 2017-07-07T20:50:30Z

Hi @akatrevorjay ,

After internal discussion:

we really appreciate the contribution, and believe that making the cpickle version configurable is a good (great even) change.
we want the change to make it to master, but we would like it to be as atomic and small as possible before we perform the merge.

Another way of saying this is that this is a rather low level library that many people depend on, and few people actually open up and modify. This means that the benefits of a refactor are rather low, whereas the risks of something breaking because of it would impact a large number of actors in a bad way.

Such a refactor would require us to probably bump a major version for the library, which doesn't make sense given the feature set change.

All of this being said, we all agree that your code is good and the new interface and naming make sense.

Course of action:

We would like to open a new PR with as little changes as possible allowing for the configurable CPickle version. We can then bump that as a minor patch.

Since you've already done a lot of the work here, we're happy to make the PR changes on our side, but if you would like to do them on yours, let us know.

Cheers!

…kleableness * origin/master: Add that we're hiring on README Bump version Update changelog Add pypy3 to travis test matrix Run full benchmark test in travis Fix flake8 issues Test against py35 and py36 Added Python 3.5 and 3.6 to the build. Have MockMemcacheClient support non-ascii strings (pinterest#169) Update getting_started.rst Fix typo in doc Close client socket if it fails to connect (pinterest#165) Don't Raise Generic Exception (pinterest#164) correct spelling mistake

akatrevorjay · 2018-06-19T05:46:04Z

@nichochar How's that? Addressed all concerns, I believe?

akatrevorjay · 2018-06-19T06:07:15Z

The failure is due to #178 , ie nothing to worry about

…kleableness * origin/master: trevor sucks Remove deprecated Python version.

akatrevorjay · 2018-07-11T22:15:02Z

Updated to latest master so it can pass tests (my other PR fixed that)

akatrevorjay · 2018-08-07T04:19:57Z

@nichochar Hello again, I'm not sure if you saw my changes? Hope you're doing well!

nichochar · 2018-08-11T17:09:49Z

Hello again, thanks for bumping! We definitely appreciate the work.

Since there is a lot of changes, and I'm no longer making big decisions related to this project, I will defer the review decision to @jogo .

In of itself, the code looks fine, the arguments against it I believe are that the project is very mature and stable, this isn't fixing any bugs but refactoring quite a bit, and losing blame history with a file rewrite. The benefits need to be measured well, and I'll defer to @jogo

jparise · 2018-11-23T18:46:44Z

Closing this in favor of the approach implemented in #190.

akatrevorjay added 2 commits June 2, 2017 18:27

Make serde pickle version configurable

93d4d39

Merge remote-tracking branch 'origin/master' into pr/pickleableness

d46ab71

* origin/master: Reverse backwards compatible change removed apidoc directory from repo add apidoc to .gitignore

akatrevorjay added 2 commits June 2, 2017 18:35

Add pickle version test

33be5f1

My VT100 requires this

e4443b8

akatrevorjay commented Jun 3, 2017

View reviewed changes

akatrevorjay added 2 commits June 2, 2017 18:42

Typo

7cfd6a4

mrclean

5316ee3

This was referenced Jun 3, 2017

Reopen PR with changes around pickle versions #159

Closed

Change the serializer API to be a single class #56

Closed

akatrevorjay added 2 commits June 2, 2017 18:53

ABC for serializers

9a4a2e7

Naming

cae7bcb

akatrevorjay mentioned this pull request Jun 3, 2017

Serialization refactor #161

Closed

akatrevorjay added 3 commits June 2, 2017 19:40

Naming

ab81cf4

Naming

61bb368

Forgot this guy

837401e

akatrevorjay mentioned this pull request Jun 3, 2017

Add support for pluggable compression #53

Closed

Typo

696df15

Fix typo that was in original

02ea2cc

nichochar approved these changes Jun 8, 2017

View reviewed changes

jogo requested changes Jun 9, 2017

View reviewed changes

nichochar requested changes Jul 7, 2017

View reviewed changes

akatrevorjay added 5 commits June 18, 2018 22:36

Rename

dc000c7

absimport

aec88e4

vt100

11a042d

life is hard sometimes

e0c9cf0

akatrevorjay added 2 commits June 18, 2018 22:43

humans are amazing

681f48e

break tab completion

62e347e

this is why we can't have nice things

a84d0a9

Merge remote-tracking branch 'origin/master' into akatrevorjay-pr/pic…

edceb08

…kleableness * origin/master: trevor sucks Remove deprecated Python version.

jparise closed this Nov 23, 2018

Make serde pickle version configurable #160

Make serde pickle version configurable #160

Conversation

akatrevorjay commented Jun 3, 2017 • edited Loading

akatrevorjay commented Jun 3, 2017

Choose a reason for hiding this comment

akatrevorjay commented Jun 3, 2017 • edited Loading

akatrevorjay commented Jun 6, 2017

nichochar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nichochar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nichochar commented Jul 7, 2017

Course of action:

akatrevorjay commented Jun 19, 2018

akatrevorjay commented Jun 19, 2018

akatrevorjay commented Jul 11, 2018

akatrevorjay commented Aug 7, 2018

nichochar commented Aug 11, 2018

jparise commented Nov 23, 2018

akatrevorjay commented Jun 3, 2017 •

edited

Loading

akatrevorjay commented Jun 3, 2017 •

edited

Loading