Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions bedrock/base/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,16 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

import logging

from django.conf import settings
from django.core.cache import caches
from django.core.cache.backends.locmem import DEFAULT_TIMEOUT, LocMemCache

from bedrock.base import metrics

logger = logging.getLogger(__name__)


class SimpleDictCache(LocMemCache):
"""A local memory cache that doesn't pickle values.
Expand Down Expand Up @@ -70,3 +76,89 @@ def incr(self, key, delta=1, version=None):
with self._lock:
self._cache[key] = new_value
return new_value


# === HYBRID CACHE BEHAVIOUR ===
#
# Our "hybrid cache" approach uses SimpleDictCache as a local, read-through
# cache on the pod, which falls back to get values from a distributed
# DB-backed cache.
# The DB-backed cache is NOT a read-through cache and only has its values
# set explicitly.

local_cache = caches["default"] # This is the SimpleDictCache
db_cache = caches["db"]


def get_from_hybrid_cache(key, default=None):
"""
Retrieve a value from the hybrid cache. First checks local cache, then falls
back to DB cache.

If found in DB cache, the value is added to local cache for faster subsequent
access.

This can be called from any code, because it does not require write access to
the DB.

:param key: The cache key to retrieve.
:param default: Default value to return if the key is not found in either cache.

:return: The cached value, or the default if the key is not found.
"""
# Check local cache
value = local_cache.get(key)
if value is not None:
return value

# Check DB cache and if it has a value, pop it into
# the local cache en route to returning the value
value = db_cache.get(key)
if value is not None:
local_cache.set(
key,
value,
timeout=settings.CACHE_TIME_SHORT,
)
return value

return default


def set_in_hybrid_cache(key, value, timeout=None):
"""
Set a value in the hybrid cache.

Writes to both the local cache and the DB cache.

IMPORTANT: this should only be called from somewhere with DB-write access -
i.e. the CMS deployment pod – if it is called from a Web deployment pod, it
will only set the local-memory cache and also log an exception, because
there will be unpredictable results if you're tryint to cache cache
Comment thread
stevejalim marked this conversation as resolved.
Outdated
something that should be available across pods -- and if you're not you
should just use the regular 'default' local-memory cache directly.

:param key: The cache key to set.
:param value: The value to cache.
:param timeout: Timeout for DB cache in seconds (local cache will use a shorter timeout by default).
"""
# Set in DB cache first, with the provided optional timeout.
# In settings we have a timeout of None, so it will never expire
# but still can be replaced (via this helper function)

try:
db_cache.set(
key,
value,
timeout=timeout,
)
except Exception as ex:
# Cope with the DB cache not being available - eg
logger.exception(f"Could not set value in DB-backed cache: {ex}")

# Set in local cache with a short timeout
local_cache.set(
key,
value,
timeout=settings.CACHE_TIME_SHORT,
)
98 changes: 98 additions & 0 deletions bedrock/base/tests/test_hybrid_cache_helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

from django.conf import settings
from django.core.cache import caches

import pytest

from bedrock.base.cache import get_from_hybrid_cache, set_in_hybrid_cache

pytestmark = [
pytest.mark.django_db,
]

local_cache = caches["default"]
db_cache = caches["db"]


@pytest.fixture(autouse=True)
def clear_caches():
print("REMOVE PRINT STATEMENT")
Comment thread
stevejalim marked this conversation as resolved.
Outdated
local_cache.clear()
db_cache.clear()


def test_hybrid_cache_get():
key = "test_key"
value = "test_value"

local_cache.set(
key,
value,
timeout=settings.CACHE_TIME_SHORT,
)
db_cache.set(
key,
value,
timeout=settings.CACHE_TIME_SHORT,
)

# Test getting from local cache directly
assert get_from_hybrid_cache(key) == value

# Test falling back to db cache and populating local cache
local_cache.clear()
assert local_cache.get(key) is None
assert db_cache.get(key) == value

assert get_from_hybrid_cache(key) == value
assert local_cache.get(key) == value
Comment thread
stevejalim marked this conversation as resolved.


def test_hybrid_cache_get_no_values_in_local_or_db_cache():
key = "test_key"

assert local_cache.get(key) is None
assert db_cache.get(key) is None
assert get_from_hybrid_cache(key) is None


def test_hybrid_cache_get__default_value():
# Test getting default value when key is not found
assert (
get_from_hybrid_cache(
"non_existent_key",
default="default_value",
)
== "default_value"
)


def test_hybrid_cache_set():
key = "test_key"
value = "test_value"
set_in_hybrid_cache(key, value)

assert local_cache.get(key) == value
assert db_cache.get(key) == value


def test_set_in_hybrid_cache_db_cache_failure(caplog, mocker):
key = "test_key_db_failure"
value = "test_value_db_failure"
timeout = 60

mocker.patch.object(
db_cache,
"set",
side_effect=Exception("Faked DB cache failure"),
)

set_in_hybrid_cache(key, value, timeout)

assert local_cache.get(key) == value
assert db_cache.get(key) is None

assert caplog.records[0].msg == "Could not set value in DB-backed cache: Faked DB cache failure"
8 changes: 7 additions & 1 deletion bedrock/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ def data_path(*args):
CACHE_TIME_MED = 60 * 60 # 1 hour
CACHE_TIME_LONG = 60 * 60 * 6 # 6 hours


CACHES = {
"default": {
"BACKEND": "bedrock.base.cache.SimpleDictCache",
Expand All @@ -101,6 +100,13 @@ def data_path(*args):
"CULL_FREQUENCY": 4, # 1/4 entries deleted if max reached
},
},
"db": {
# Intended for use as a slower – but distributed - cache
# See bedrock.base.cache.get_from_hybrid_cache and set_in_hybrid_cache
"LOCATION": "hybrid_cache_db_table", # name of DB table to be used
"BACKEND": "django.core.cache.backends.db.DatabaseCache",
"TIMEOUT": None, # cached items will not expire

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want this to be forever? Have we considered a long timeout so they eventually do get purged? No expiry ever seems like this table would potentially bloat over time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the default timeout for the cache, so that we can support the idea of never-expiring things, but set_in_hybrid_cache does allow a timeout to be set if we wanted to. If we had a default here, my understanding is that if we did set timeout=None a call to set something in the cache, it would result in the defualt being used instead of None

Related to this, though, I've just thought about providing more flexibility in speccing cache times for db and locmem cacheing when using set_in_hybrid_cache, so I'll re-request an R after pushing that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at the cache backends in Django, they use django.core.cache.backends.base.DEFAULT_TIMEOUT to differentiate between None and the default.

The backends also define a get_backend_timeout that could be useful?

My thought is we should use a very long expiry as the default, even if it's like 1 year or something, just so old unused cache does get purged. And have to explicitly set None to have no timeout at all, b/c that feels like it should be on purpose.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, great - if it is possible to override the default with None, then that's great. Will make the change now

},
}

# Logging
Expand Down
5 changes: 5 additions & 0 deletions bin/export-db-to-sqlite.sh
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,11 @@ PROD_DETAILS_STORAGE=product_details.storage.PDFileStorage \

check_status_and_handle_failure "Running Django migrations"

PROD_DETAILS_STORAGE=product_details.storage.PDFileStorage \
python manage.py createcachetable || all_well=false

check_status_and_handle_failure "Creating cache table migrations"

# We want to use all the data from the JSON, so let's drop the rows
# that have been automatically populated during migrate, including all the Wagtail
# ones, except for wagtailsearch's tables because there's a virtual table that
Expand Down