Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enforcing pickle to use copy of dict while serializing extra context of the notification #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jumpjam
Copy link

@jumpjam jumpjam commented Apr 15, 2014

This fixes a type of notification extra context pickling in our case (remove was failing as key not found).

Example that satisfies the condition:
import pickle
x = {'order_number': 'X', 'deal_url': 'J'}

pickle.dumps(x)
pickle.dumps(pickle.loads(pickle.dumps(x)))
pickle.dumps(pickle.loads(pickle.dumps(pickle.loads(pickle.dumps(x)))))

Where 1 and 3 are same but second is different.

@tbarbugli tbarbugli closed this Apr 15, 2014
@tbarbugli tbarbugli reopened this Apr 15, 2014
@tschellenbach
Copy link
Owner

Which feed class and with which storage backend did you run into this problem?

Thanks for contributing, we really appreciate it.

@tschellenbach
Copy link
Owner

Part of the problem seems to be that:
https://github.com/tschellenbach/Feedly/blob/master/feedly/storage/redis/timeline_storage.py#L91

Removes by value instead of by key. Not sure why this is.

@tschellenbach
Copy link
Owner

Think we could easily use http://redis.io/commands/zremrangebyscore over there

@jumpjam
Copy link
Author

jumpjam commented Apr 15, 2014

Hi,

I have used current redis version with following notification class:

class NotificationFeedly(RedisNotificationFeed):
key_format = 'custom_feed:notification:%(user_id)s'

Debugged down to zrem which returned 0 as value is not found.

I also agree using rembyscore which might be a reliable and faster solution.
We will keep fork and switch back to main rep in case any changes on serialization or zrem.

Thank you

@Atorich
Copy link

Atorich commented Nov 21, 2015

Solution #1191a64 doesnt resolve a case with nested dicts. Recursive algorithm will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants