Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#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 Mariusz Felisiak)

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:1 by Simon Charette, 3 years ago

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 other pymemcache based backend don't behave this way and switching to the backend provided by Django might cause a change in behaviour.

Last edited 3 years ago by Simon Charette (previous) (diff)

comment:2 by Mariusz Felisiak, 3 years ago

Cc: pope1ni added
Component: UncategorizedCore (Cache system)

comment:3 by Nick Pope, 3 years ago

Component: Core (Cache system)Documentation
Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/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.

comment:4 by Simon Charette, 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?

Last edited 3 years ago by Simon Charette (previous) (diff)

comment:5 by Nishant Sagar, 3 years ago

Owner: changed from nobody to Nishant Sagar
Status: newassigned

comment:6 by Nishant Sagar, 3 years ago

Description: modified (diff)
Last edited 3 years ago by Nishant Sagar (previous) (diff)

in reply to:  4 comment:7 by Nishant Sagar, 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 Mariusz Felisiak, 3 years ago

Description: modified (diff)

comment:9 by Adam Johnson, 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 Mariusz Felisiak, 2 years ago

Owner: Nishant Sagar removed
Status: assignednew
Summary: PyMemcacheCache uses default_noreply=False although pymemcache recommends to set to TrueDocument default options set by PyMemcacheCache.

comment:12 by Pablo Montepagano, 2 years ago

Owner: set to Pablo Montepagano
Status: newassigned

comment:13 by Mariusz Felisiak, 2 years ago

Has patch: set

comment:14 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:15 by Mariusz Felisiak, 2 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:16 by GitHub <noreply@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In bb2c5f69:

Fixed #32749 -- Doc'd PyMemcacheCache defaults.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 72358f01:

[4.1.x] Fixed #32749 -- Doc'd PyMemcacheCache defaults.

Backport of bb2c5f69f47466fa52f3cf2727d10b3ebd79a4da from main

Note: See TracTickets for help on using tickets.
Back to Top