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

fix django2.0 #179

Merged
merged 14 commits into from
Aug 15, 2018
Merged

fix django2.0 #179

merged 14 commits into from
Aug 15, 2018

Conversation

opapy
Copy link
Contributor

@opapy opapy commented Jul 10, 2018

Failed set_many with django2.0.

>>> from django.core.cache import cache
>>> cache.set_many({'aa': 1})
Traceback (most recent call last):
  File "/usr/lib/python3.6/code.py", line 91, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
  File "/usr/local/lib/python3.6/dist-packages/ddtrace/contrib/django/cache.py", line 65, in wrapped
    return method(*args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/django_elastipymemcache/memcached.py", line 43, in wrapper
    return f(self, *args, **kwds)
  File "/usr/local/lib/python3.6/dist-packages/django_elastipymemcache/memcached.py", line 135, in set_many
    return super(ElastiPyMemCache, self).set_many(*args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/django/core/cache/backends/memcached.py", line 144, in set_many
    return [original_keys[k] for k in failed_keys]
TypeError: 'bool' object is not iterable

Release Note: https://docs.djangoproject.com/en/2.0/releases/2.0/#cache
Code: https://github.com/django/django/blob/338f741c5eb6b91118f6a6b7c34b5e9b47a5661d/django/core/cache/backends/memcached.py#L131-L139

Since django 2.0, the return value of set_many now expects a list of keys of failed objects in store.

It should be implemented at the cache backend, but python-memcached and pylibmc return list objects, so I tried it accordingly.

@jogo
Copy link
Contributor

jogo commented Jul 11, 2018

Looks good, although this does change the behavior of the API so we should make sure to document that in the changelog. but the change behavior is fairly minor and looks reasonable. This doesn't seem to fully implement django's expected behavior of returning a list of failed keys, is that expected?

@opapy
Copy link
Contributor Author

opapy commented Jul 11, 2018

hi @jogo
Sure, this implementation returns a list of keys failed to store in HashClient only.

PooledClient and Client always return empty list.
Client and PooledClient is always True, if it fails it returns an exception, so I thought whether this implementation would be a problem.

@@ -261,11 +261,11 @@ def set_many(self, values, *args, **kwargs):
new_args.insert(0, values)
result = self._safely_run_func(
client,
client.set_many, False, *new_args, **kwargs
client.set_many, values.keys(), *new_args, **kwargs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this means, if any of these keys fail, we mark all as failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!
That's true. I will fix it.

@Surgo
Copy link

Surgo commented Aug 14, 2018

@jogo any update on this issue?
This PR is blocker of django 2.0 migrations...

Copy link
Contributor

@jogo jogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, one comment

or none of the keys have been successfully set. If noreply is True
then a successful return does not guarantee that any keys were
successfully set (just that the keys were successfully sent).
Empty list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this now return a list of failed sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx! i fixed it.

@opapy
Copy link
Contributor Author

opapy commented Aug 15, 2018

@nichochar
Copy link
Collaborator

Job has been restarted

@jogo jogo merged commit 88e5bf5 into pinterest:master Aug 15, 2018
@jogo
Copy link
Contributor

jogo commented Aug 15, 2018

Thanks looks good. Will work on releasing this sometime next week as a major version bump.

@opapy
Copy link
Contributor Author

opapy commented Aug 16, 2018

@jogo
thunks for review!

@Surgo
Copy link

Surgo commented Sep 4, 2018

@jogo When release it?

@jparise
Copy link
Collaborator

jparise commented Sep 4, 2018

@Surgo I think we're on track for a 1.5.0 release pretty soon. I'd like to see #184 reviewed and merged first, and then we can start preparing a new release.

@jogo jogo mentioned this pull request Sep 7, 2018
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.

5 participants