#32749 closed Cleanup/optimization (fixed)
Document default options set by PyMemcacheCache.
Reported by: | yakirsudry | Owned by: | Pablo Montepagano |
---|---|---|---|
Component: | Documentation | Version: | 3.2 |
Severity: | Normal | Keywords: | pymemcache PyMemcacheCache |
Cc: | pope1ni, אורי | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
I'm not sure why would we do that, since changing to True will slightly speed up the usage of the cache
Change History (17)
comment:2 by , 3 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Core (Cache system) |
comment:3 by , 3 years ago
Component: | Core (Cache system) → Documentation |
---|---|
Easy pickings: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
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 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.
follow-up: 7 comment:4 by , 3 years ago
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 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 enable 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?
comment:5 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 3 years ago
Description: | modified (diff) |
---|
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:8 by , 3 years ago
Description: | modified (diff) |
---|
comment:9 by , 3 years ago
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.
comment:10 by , 3 years ago
Cc: | added |
---|
comment:11 by , 2 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
Summary: | PyMemcacheCache uses default_noreply=False although pymemcache recommends to set to True → Document default options set by PyMemcacheCache. |
comment:12 by , 2 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:14 by , 2 years ago
Patch needs improvement: | set |
---|
comment:15 by , 2 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Parallel conversation on Github. Nick mentioned that it was done to make the backend a drop in replacement for 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 as
django-pymemcache
or otherpymemcache
based backend don't behave this way and switching to the backend provided by Django might cause a change in behaviour.