[Django] #32749: PyMemcacheCache uses default_noreply=False although pymemcache recommends to set to True
Django
to set to True
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
yakirsudry |
Type: Bug | Status: new
Component: | Version: 3.2
Uncategorized | Keywords: pymemcache
Severity: Normal | PyMemcacheCache
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I'm not sure why would we do that, since changing to True will slightly
speed up the usage of the cache
--
Ticket URL: <https://code.djangoproject.com/ticket/32749>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
to set to True
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Uncategorized | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage:
PyMemcacheCache | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
[https://github.com/django/django/pull/13310#discussion_r632986159
Parallel conversation on Github]. Nick mentioned that it was done to make
the backend a drop in replacement with other MC backends but I'm curious
to see if any tests fail without this override. At the least we should
mention in the docs that these defaults are set for the `pymemcache`
backend as `django-pymemcache` doesn't behave this way.
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:1>
Django
to set to True
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage:
PyMemcacheCache | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Nick Pope (added)
* component: Uncategorized => Core (Cache system)
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:2>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Nick Pope):
* type: Bug => Cleanup/optimization
* component: Core (Cache system) => Documentation
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Thank you for dredging up my comment Simon.
As mentioned there, I was trying to ensure that the new `pymemcache`
backend was compatible with the expectations of Django by
[https://github.com/django/django/blob/0851933cba7b40e22f5e424c95763dbc27c40aa9/django/core/cache/backends/memcached.py#L238-L240
setting various options].
If I remove `'default_noreply': False,` then a heap of tests fail because
`pymemcache` always returns `True` for some operations:
{{{
$ python runtests.py --settings=test_sqlite --parallel=1 --timing
cache.tests.PyMemcacheCacheTests
Testing against Django installed in '/home/pope1ni/Sources/django/django'
Found 58 tests.
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
F....F..F......ss.........F..F........................F.s.
======================================================================
FAIL: test_add (cache.tests.PyMemcacheCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pope1ni/Sources/django/tests/cache/tests.py", line 306, in
test_add
self.assertIs(cache.add("addkey1", "newvalue"), False)
AssertionError: True is not False
======================================================================
FAIL: test_cache_versioning_add (cache.tests.PyMemcacheCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pope1ni/Sources/django/tests/cache/tests.py", line 767, in
test_cache_versioning_add
self.assertIs(cache.add('answer1', 37, version=2), False)
AssertionError: True is not False
======================================================================
FAIL: test_cache_versioning_get_set_many
(cache.tests.PyMemcacheCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pope1ni/Sources/django/tests/cache/tests.py", line 900, in
test_cache_versioning_get_set_many
self.assertEqual(cache.get_many(['ford3', 'arthur3'], version=2),
{'ford3': 37, 'arthur3': 42})
AssertionError: {'ford3': 37} != {'ford3': 37, 'arthur3': 42}
- {'ford3': 37}
+ {'arthur3': 42, 'ford3': 37}
======================================================================
FAIL: test_delete_nonexistent (cache.tests.PyMemcacheCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pope1ni/Sources/django/tests/cache/tests.py", line 344, in
test_delete_nonexistent
self.assertIs(cache.delete('nonexistent_key'), False)
AssertionError: True is not False
======================================================================
FAIL: test_forever_timeout (cache.tests.PyMemcacheCacheTests)
Passing in None into timeout results in a value that is cached forever
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pope1ni/Sources/django/tests/cache/tests.py", line 597, in
test_forever_timeout
self.assertIs(cache.add('key1', 'new eggs', None), False)
AssertionError: True is not False
======================================================================
FAIL: test_touch (cache.tests.PyMemcacheCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pope1ni/Sources/django/tests/cache/tests.py", line 484, in
test_touch
self.assertIs(cache.touch('nonexistent'), False)
AssertionError: True is not False
----------------------------------------------------------------------
Ran 58 tests in 12.032s
FAILED (failures=6, skipped=3)
Destroying test database for alias 'default'...
Total database setup took 0.090s
Creating 'default' took 0.090s
Total database teardown took 0.000s
Total run took 12.187s
}}}
It is perfectly possible for developers to add `'default_noreply': True,`
to their `OPTIONS` for the cache to override this.
I find it a strange default. Yes, it is faster, but probably less safe -
see the failure for `test_cache_versioning_get_set_many` above.
We can add an admonition to state what options are being set by default
for `pymemcache`, but I don't feel that we need to change the behaviour.
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:3>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Thanks for the extra details Nick.
In the light of these failures I think the only way we could get tests
passing/ensure the backend is fully functional without turning reply on
per client would be to adjust all the methods that require a reply from MC
to work properly to explicitly pass `noreply=False` but I'm not sure there
would be much benefit from that.
Documenting to turn the option as is would simply break the backend so I'm
not convinced we should even do that anymore. Were you aware of that
yakirsudry?
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:4>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Sagar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Nishant Sagar
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:5>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Sagar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Old description:
> I'm not sure why would we do that, since changing to True will slightly
> speed up the usage of the cache
New description:
I checked the pymemcached documentation and it says noreply will not read
errors returned from the memcached server.
If a function with noreply=True causes an error on the server, it will
still succeed and your next call which reads a response from memcached may
fail unexpectedly.
pymemcached will try to catch and stop you from sending malformed inputs
to memcached, but if you are having unexplained errors, setting
noreply=False may help you troubleshoot the issue.
What do we supposed to do now?
PS- I'm new to this project so can anyone give me some suggestion to fix
this issue
--
Comment (by Nishant Sagar):
I checked the pymemcached documentation and it says noreply will not read
errors returned from the memcached server.
If a function with noreply=True causes an error on the server, it will
still succeed and your next call which reads a response from memcached may
fail unexpectedly.
pymemcached will try to catch and stop you from sending malformed inputs
to memcached, but if you are having unexplained errors, setting
noreply=False may help you troubleshoot the issue.
What do we supposed to do now?
PS- I'm new to this project so can anyone give me some suggestion to fix
this issue
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:6>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Sagar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Old description:
> I checked the pymemcached documentation and it says noreply will not
> read errors returned from the memcached server.
>
> If a function with noreply=True causes an error on the server, it will
> still succeed and your next call which reads a response from memcached
> may fail unexpectedly.
>
> pymemcached will try to catch and stop you from sending malformed inputs
> to memcached, but if you are having unexplained errors, setting
> noreply=False may help you troubleshoot the issue.
>
> What do we supposed to do now?
>
> PS- I'm new to this project so can anyone give me some suggestion to fix
> this issue
New description:
--
Comment (by Nishant Sagar):
I checked the pymemcached documentation and it says noreply will not read
errors returned from the memcached server.
If a function with noreply=True causes an error on the server, it will
still succeed and your next call which reads a response from memcached may
fail unexpectedly.
pymemcached will try to catch and stop you from sending malformed inputs
to memcached, but if you are having unexplained errors, setting
noreply=False may help you troubleshoot the issue.
What do we supposed to do now?
PS- I'm new to this project so can anyone give me some suggestion to fix
this issue
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:7>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Sagar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Description changed by Mariusz Felisiak:
Old description:
New description:
I'm not sure why would we do that, since changing to True will slightly
speed up the usage of the cache
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:8>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Sagar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Adam Johnson):
> We can add an admonition to state what options are being set by default
for pymemcache, but I don't feel that we need to change the behaviour.
+1 to this
> adjust all the methods that require a reply from MC to work properly to
explicitly pass noreply=False but I'm not sure there would be much benefit
from that.
Whether the reply is required depends on context... e.g. in the context of
tests, we check the status of operations, but user code may not care.
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:9>
Django
to set to True
-------------------------------------+-------------------------------------
Type: | Sagar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: אורי (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: yakirsudry | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: Nishant Sagar => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Montepagano
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: (none) => Pablo Montepagano
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: yakirsudry | Owner: Pablo
Type: | Montepagano
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15796 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: yakirsudry | Owner: Pablo
Type: | Montepagano
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
Keywords: pymemcache | Triage Stage: Accepted
PyMemcacheCache |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: yakirsudry | Owner: Pablo
Type: | Montepagano
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 3.2
Severity: Normal | Resolution:
PyMemcacheCache | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: yakirsudry | Owner: Pablo
Type: | Montepagano
Component: Documentation | Version: 3.2
Severity: Normal | Resolution: fixed
Keywords: pymemcache | Triage Stage: Ready for
PyMemcacheCache | checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"bb2c5f69f47466fa52f3cf2727d10b3ebd79a4da" bb2c5f69]:
{{{
#!CommitTicketReference repository=""
revision="bb2c5f69f47466fa52f3cf2727d10b3ebd79a4da"
Fixed #32749 -- Doc'd PyMemcacheCache defaults.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: yakirsudry | Owner: Pablo
Type: | Montepagano
Cleanup/optimization | Status: closed
Component: Documentation | Version: 3.2
Severity: Normal | Resolution: fixed
Keywords: pymemcache | Triage Stage: Ready for
PyMemcacheCache | checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"72358f0110f3967d95f077986e70e5c8080e9962" 72358f01]:
{{{
#!CommitTicketReference repository=""
revision="72358f0110f3967d95f077986e70e5c8080e9962"
[4.1.x] Fixed #32749 -- Doc'd PyMemcacheCache defaults.
Backport of bb2c5f69f47466fa52f3cf2727d10b3ebd79a4da from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32749#comment:17>