Only use in-memory cache for database settings, set ttl=5 #12166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlanCoding
force-pushed
the
thread_hot
branch
2 times, most recently
from
d3f75a4
to
3156c49
Compare
|
This is blocked on a particularly nasty migration error on initial migration. |
|
That recursion error looks like it's been fixed, running more tests now. |
|
I believe this is still relevant, but needs to be rebased and re-tested on top of #11373 |
|
Clarifying #12084 |
Make necessary adjustments to monkeypatch as it is very vunerable to recursion Clear cache for each request Clear cache if a setting is changed
|
Testing is looking good, now I just plan to write a short knowledge base article on general cache knowledge. |
AlanCoding
changed the title
AlanCoding
commented
May 10, 2022
|
|
||
| val = None | ||
| if 'migrate' not in sys.argv: | ||
| if (isinstance(attr, str)) and (attr in DEFAULTS) and (not attr.startswith('_')): |
There was a problem hiding this comment.
@jbradberry I'm removing the exception for migrations here because it is handled elsewhere now - by not initializing the settings wrapper when this condition is met. The other changes are to be more protective in __getattribute__ code, which is scary.
jbradberry
reviewed
May 10, 2022
| @@ -26,6 +26,11 @@ | |||
| perf_logger = logging.getLogger('awx.analytics.performance') | |||
|
|
|||
|
|
|||
| class SettingsCacheMiddleware(MiddlewareMixin): | |||
| def process_request(self, request): | |||
| settings._awx_conf_memoizedcache.clear() | |||
There was a problem hiding this comment.
I don't get the reasoning of doing this here.
There was a problem hiding this comment.
Any given automated script could do this:
- make a PATCH to
/api/v2/settings/jobs/, applying new values - right after that, make a GET to
/api/v2/settings/jobs/
Regardless of what uWSGI worker processes the second (GET) request, it will be under the 5 second ttl threshold. That means that it would give the values from before the PATCH. The automated script will then conclude that the PATCH it made did not take effect.
Now replace the generalized "automated script" with out integration tests, and you see the immediate reason this was needed.
jbradberry
approved these changes
May 10, 2022
rebeccahhh
approved these changes
May 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This is a speculative replacement for #12095, and maybe #5765 for good measure.
EDIT: due to unexpected developments, this does not replace 12095, but it does resolve 5765
Made a writeup on this here:
https://gist.github.com/AlanCoding/3bff66c63db36ae7cb8c326df2c38e29
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
The
__getattr__method here will be called for any Django setting. The_get_localis only called for settings registered via our database settings system. For terminology, we call settings which are not registered "file-based settings".It doesn't make any sense to cache file-based settings. They are already static values. This in-memory cache is not only worthless for file-based settings, it actively throws errors. This will even happen when the
ttlvalue is 0, so someone probably thought that this was equivalent to turning the caching off, but it's not.I didn't 100% need to introduce a new method here, but doing this avoids including the
validatekeyword argument in the cache key, which is my preference. It might also reduce the test churn from the change.