Andal.LND/edx-platform
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #35185 from openedx/feanil/blockstructure_cache

Browse Source 
feat!: Drop the block_structure.storage_backing_for_cache WaffleSwitch
This commit is contained in:
Feanil Patel
2024-11-06 10:52:47 -05:00
committed by GitHub
parent 8c61ad6f4d 4768568b5a
commit 5904b54378
23 changed files with 166 additions and 244 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
openedx/core/djangoapps/content/block_structure/__init__.py
View File

@@ -59,4 +59,10 @@ contained transformers within them.
Note: A partial subset (as an ordered list) of the registered
transformers can be requested during the Transform phase, allowing
the client to manipulate exactly which transformers to call.
Links to Other Block Structure Related Documentation:
* https://openedx.atlassian.net/wiki/spaces/AC/pages/154861855/Block+Structure+Cache+Invalidation+Proposal
* https://openedx.atlassian.net/wiki/spaces/AC/pages/34734111/Course+Block+Transformers
* https://openedx.atlassian.net/wiki/spaces/AC/pages/41910826/Course+Blocks+API+Storage+Cache+Requirements
"""

31
openedx/core/djangoapps/content/block_structure/config/__init__.py
View File

@@ -9,37 +9,6 @@ from openedx.core.lib.cache_utils import request_cached
from .models import BlockStructureConfiguration
# Switches
# .. toggle_name: block_structure.storage_backing_for_cache
# .. toggle_implementation: WaffleSwitch
# .. toggle_default: False
# .. toggle_description: When enabled, block structures are stored in a more permanent storage,
# like a database, which provides an additional backup for cache misses, instead having them
# regenerated. The regenration of block structures is a time consuming process. Therefore,
# enabling this switch is recommended for Production.
# .. toggle_warning: Depends on `BLOCK_STRUCTURES_SETTINGS['STORAGE_CLASS']` and
# `BLOCK_STRUCTURES_SETTINGS['STORAGE_KWARGS']`.
# This switch will likely be deprecated and removed.
# The annotation will be updated with the DEPR ticket once that process has started.
# .. toggle_use_cases: temporary
# .. toggle_creation_date: 2017-02-23
# .. toggle_target_removal_date: 2017-05-23
# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/14512,
# https://github.com/openedx/edx-platform/pull/14770,
# https://openedx.atlassian.net/browse/DEPR-145
STORAGE_BACKING_FOR_CACHE = WaffleSwitch(
"block_structure.storage_backing_for_cache", __name__
)
def enable_storage_backing_for_cache_in_request():
"""
Manually override the value of the STORAGE_BACKING_FOR_CACHE switch in the context of the request.
This function should not be replicated, as it accesses a protected member, and it shouldn't.
"""
# pylint: disable=protected-access
STORAGE_BACKING_FOR_CACHE._cached_switches[STORAGE_BACKING_FOR_CACHE.name] = True
@request_cached()
def num_versions_to_keep():

12
openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py
View File

@@ -10,7 +10,6 @@ from django.core.management.base import BaseCommand
import openedx.core.djangoapps.content.block_structure.api as api
import openedx.core.djangoapps.content.block_structure.store as store
import openedx.core.djangoapps.content.block_structure.tasks as tasks
from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request
from openedx.core.lib.command_utils import (
get_mutually_exclusive_required_option,
parse_course_keys,
@@ -75,12 +74,6 @@ class Command(BaseCommand):
default=0,
type=int,
)
parser.add_argument(
'--with_storage',
help='Store the course blocks in Storage, overriding value of the storage_backing_for_cache waffle switch',
action='store_true',
default=False,
)
def handle(self, *args, **options):
@@ -129,9 +122,6 @@ class Command(BaseCommand):
"""
Generates course blocks for the given course_keys per the given options.
"""
if options.get('with_storage'):
enable_storage_backing_for_cache_in_request()
for course_key in course_keys:
try:
self._generate_for_course(options, course_key)
@@ -150,7 +140,7 @@ class Command(BaseCommand):
action = tasks.update_course_in_cache_v2 if options.get('force_update') else tasks.get_course_in_cache_v2
task_options = {'routing_key': options['routing_key']} if options.get('routing_key') else {}
result = action.apply_async(
kwargs=dict(course_id=str(course_key), with_storage=options.get('with_storage')),
kwargs=dict(course_id=str(course_key)),
**task_options
)
log.info('BlockStructure: ENQUEUED generating for course: %s, task_id: %s.', course_key, result.id)

7
openedx/core/djangoapps/content/block_structure/management/commands/tests/test_generate_course_blocks.py
View File

@@ -85,15 +85,8 @@ class TestGenerateCourseBlocks(ModuleStoreTestCase):
assert mock_update_from_store.call_count == (self.num_courses if force_update else 0)
def test_one_course(self):
self._assert_courses_not_in_block_cache(*self.course_keys)
self.command.handle(courses=[str(self.course_keys[0])])
self._assert_courses_in_block_cache(self.course_keys[0])
self._assert_courses_not_in_block_cache(*self.course_keys[1:])
self._assert_courses_not_in_block_storage(*self.course_keys)
def test_with_storage(self):
self.command.handle(with_storage=True, courses=[str(self.course_keys[0])])
self._assert_courses_in_block_cache(self.course_keys[0])
self._assert_courses_in_block_storage(self.course_keys[0])
self._assert_courses_not_in_block_storage(*self.course_keys[1:])

4
openedx/core/djangoapps/content/block_structure/models.py
View File

@@ -74,7 +74,6 @@ def _bs_model_storage():
# .. setting_default: None
# .. setting_description: Specifies the storage used for storage-backed block structure cache.
# For more information, check https://github.com/openedx/edx-platform/pull/14571.
# .. setting_warnings: Depends on `block_structure.storage_backing_for_cache`.
storage_class = settings.BLOCK_STRUCTURES_SETTINGS.get('STORAGE_CLASS')
# .. setting_name: BLOCK_STRUCTURES_SETTINGS['STORAGE_KWARGS']
@@ -82,8 +81,7 @@ def _bs_model_storage():
# .. setting_description: Specifies the keyword arguments needed to setup the storage, which
# would be used for storage-backed block structure cache.
# For more information, check https://github.com/openedx/edx-platform/pull/14571.
# .. setting_warnings: Depends on `BLOCK_STRUCTURES_SETTINGS['STORAGE_CLASS']` and on
# `block_structure.storage_backing_for_cache`.
# .. setting_warnings: Depends on `BLOCK_STRUCTURES_SETTINGS['STORAGE_CLASS']`
storage_kwargs = settings.BLOCK_STRUCTURES_SETTINGS.get('STORAGE_KWARGS', {})
return get_storage(storage_class, **storage_kwargs)

67
openedx/core/djangoapps/content/block_structure/store.py
View File

@@ -19,26 +19,6 @@ from .transformer_registry import TransformerRegistry
logger = getLogger(__name__) # pylint: disable=C0103
class StubModel:
"""
Stub model to use when storage backing is disabled.
By using this stub, we eliminate the need for extra
conditional statements in the code.
"""
def __init__(self, root_block_usage_key):
self.data_usage_key = root_block_usage_key
def __str__(self):
return str(self.data_usage_key)
def delete(self):
"""
Noop delete method.
"""
pass # lint-amnesty, pylint: disable=unnecessary-pass
class BlockStructureStore:
"""
Storage for BlockStructure objects.
@@ -120,13 +100,12 @@ class BlockStructureStore:
Returns whether the data in storage for the given key is
already up-to-date with the version in the given modulestore.
"""
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
try:
bs_model = self._get_model(root_block_usage_key)
root_block = modulestore.get_item(root_block_usage_key)
return self._version_data_of_model(bs_model) == self._version_data_of_block(root_block)
except BlockStructureNotFound:
pass
try:
bs_model = self._get_model(root_block_usage_key)
root_block = modulestore.get_item(root_block_usage_key)
return self._version_data_of_model(bs_model) == self._version_data_of_block(root_block)
except BlockStructureNotFound:
pass
return False
@@ -134,26 +113,20 @@ class BlockStructureStore:
"""
Returns the model associated with the given key.
"""
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
return BlockStructureModel.get(root_block_usage_key)
else:
return StubModel(root_block_usage_key)
return BlockStructureModel.get(root_block_usage_key)
def _update_or_create_model(self, block_structure, serialized_data):
"""
Updates or creates the model for the given block_structure
and serialized_data.
"""
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
root_block = block_structure[block_structure.root_block_usage_key]
bs_model, _ = BlockStructureModel.update_or_create(
serialized_data,
data_usage_key=block_structure.root_block_usage_key,
**self._version_data_of_block(root_block)
)
return bs_model
else:
return StubModel(block_structure.root_block_usage_key)
root_block = block_structure[block_structure.root_block_usage_key]
bs_model, _ = BlockStructureModel.update_or_create(
serialized_data,
data_usage_key=block_structure.root_block_usage_key,
**self._version_data_of_block(root_block)
)
return bs_model
def _add_to_cache(self, serialized_data, bs_model):
"""
@@ -186,9 +159,6 @@ class BlockStructureStore:
Raises:
BlockStructureNotFound if not found.
"""
if not config.STORAGE_BACKING_FOR_CACHE.is_enabled():
raise BlockStructureNotFound(bs_model.data_usage_key)
return bs_model.get_serialized_data()
def _serialize(self, block_structure):
@@ -226,14 +196,9 @@ class BlockStructureStore:
def _encode_root_cache_key(bs_model):
"""
Returns the cache key to use for the given
BlockStructureModel or StubModel.
BlockStructureModel.
"""
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
return str(bs_model)
return "v{version}.root.key.{root_usage_key}".format(
version=str(BlockStructureBlockData.VERSION),
root_usage_key=str(bs_model.data_usage_key),
)
return str(bs_model)
@staticmethod
def _version_data_of_block(root_block):

9
openedx/core/djangoapps/content/block_structure/tasks.py
View File

@@ -14,7 +14,6 @@ from opaque_keys.edx.keys import CourseKey
from xmodule.capa.responsetypes import LoncapaProblemError
from openedx.core.djangoapps.content.block_structure import api
from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request
from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order
log = logging.getLogger('edx.celery.task')
@@ -43,8 +42,6 @@ def update_course_in_cache_v2(self, **kwargs):
Updates the course blocks (mongo -> BlockStructure) for the specified course.
Keyword Arguments:
course_id (string) - The string serialized value of the course key.
with_storage (boolean) - Whether or not storage backing should be
enabled for the generated block structure(s).
"""
_update_course_in_cache(self, **kwargs)
@@ -62,8 +59,6 @@ def _update_course_in_cache(self, **kwargs):
"""
Updates the course blocks (mongo -> BlockStructure) for the specified course.
"""
if kwargs.get('with_storage'):
enable_storage_backing_for_cache_in_request()
_call_and_retry_if_needed(self, api.update_course_in_cache, **kwargs)
@@ -74,8 +69,6 @@ def get_course_in_cache_v2(self, **kwargs):
Gets the course blocks for the specified course, updating the cache if needed.
Keyword Arguments:
course_id (string) - The string serialized value of the course key.
with_storage (boolean) - Whether or not storage backing should be
enabled for any generated block structure(s).
"""
_get_course_in_cache(self, **kwargs)
@@ -93,8 +86,6 @@ def _get_course_in_cache(self, **kwargs):
"""
Gets the course blocks for the specified course, updating the cache if needed.
"""
if kwargs.get('with_storage'):
enable_storage_backing_for_cache_in_request()
_call_and_retry_if_needed(self, api.get_course_in_cache, **kwargs)

6
openedx/core/djangoapps/content/block_structure/tests/helpers.py
View File

@@ -274,10 +274,14 @@ class ChildrenMapTestMixin:
# create empty block structure
block_structure = block_structure_cls(root_block_usage_key=self.block_key_factory(0))
# _add_relation
# _add_relation and blocks
for parent, children in enumerate(children_map):
if isinstance(block_structure, BlockStructureBlockData):
block_structure._get_or_create_block(self.block_key_factory(parent)) # pylint: disable=protected-access
for child in children:
block_structure._add_relation(self.block_key_factory(parent), self.block_key_factory(child)) # pylint: disable=protected-access
if isinstance(block_structure, BlockStructureBlockData):
block_structure._get_or_create_block(self.block_key_factory(child)) # pylint: disable=protected-access
return block_structure
def get_parents_map(self, children_map):

18
openedx/core/djangoapps/content/block_structure/tests/test_factory.py
View File

@@ -5,6 +5,7 @@ Tests for block_structure_factory.py
import pytest
from django.test import TestCase
from opaque_keys.edx.keys import CourseKey
from xmodule.modulestore.exceptions import ItemNotFoundError
from ..exceptions import BlockStructureNotFound
@@ -18,14 +19,22 @@ class TestBlockStructureFactory(TestCase, ChildrenMapTestMixin):
Tests for BlockStructureFactory
"""
def block_key_factory(self, block_id):
"""
Returns a usage_key object for the given block_id.
This overrides the method in the ChildrenMapTestMixin.
"""
return CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", str(block_id))
def setUp(self):
super().setUp()
self.children_map = self.SIMPLE_CHILDREN_MAP
self.modulestore = MockModulestoreFactory.create(self.children_map, self.block_key_factory)
def test_from_modulestore(self):
usage_key = CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", "0")
block_structure = BlockStructureFactory.create_from_modulestore(
root_block_usage_key=0, modulestore=self.modulestore
root_block_usage_key=usage_key, modulestore=self.modulestore
)
self.assert_block_structure(block_structure, self.children_map)
@@ -48,15 +57,18 @@ class TestBlockStructureFactory(TestCase, ChildrenMapTestMixin):
def test_from_cache_none(self):
store = BlockStructureStore(MockCache())
# Non-existent usage key
usage_key = CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", "0")
with pytest.raises(BlockStructureNotFound):
BlockStructureFactory.create_from_store(
root_block_usage_key=0,
root_block_usage_key=usage_key,
block_structure_store=store,
)
def test_new(self):
usage_key = CourseKey.from_string("course-v1:org+course+run").make_usage_key("html", "0")
block_structure = BlockStructureFactory.create_from_modulestore(
root_block_usage_key=0, modulestore=self.modulestore
root_block_usage_key=usage_key, modulestore=self.modulestore
)
new_structure = BlockStructureFactory.create_new(
block_structure.root_block_usage_key,

26
openedx/core/djangoapps/content/block_structure/tests/test_manager.py
View File

@@ -5,10 +5,8 @@ Tests for manager.py
import pytest
import ddt
from django.test import TestCase
from edx_toggles.toggles.testutils import override_waffle_switch
from ..block_structure import BlockStructureBlockData
from ..config import STORAGE_BACKING_FOR_CACHE
from ..exceptions import UsageKeyNotInBlockStructure
from ..manager import BlockStructureManager
from ..transformers import BlockStructureTransformers
@@ -177,20 +175,18 @@ class TestBlockStructureManager(UsageKeyFactoryMixin, ChildrenMapTestMixin, Test
self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
assert TestTransformer1.collect_call_count == 1
@ddt.data(True, False)
def test_update_collected_if_needed(self, with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
with mock_registered_transformers(self.registered_transformers):
assert TestTransformer1.collect_call_count == 0
def test_update_collected_if_needed(self):
with mock_registered_transformers(self.registered_transformers):
assert TestTransformer1.collect_call_count == 0
self.bs_manager.update_collected_if_needed()
assert TestTransformer1.collect_call_count == 1
self.bs_manager.update_collected_if_needed()
assert TestTransformer1.collect_call_count == 1
self.bs_manager.update_collected_if_needed()
expected_count = 1 if with_storage_backing else 2
assert TestTransformer1.collect_call_count == expected_count
self.bs_manager.update_collected_if_needed()
expected_count = 1
assert TestTransformer1.collect_call_count == expected_count
self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
def test_get_collected_transformer_version(self):
self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)
@@ -212,8 +208,8 @@ class TestBlockStructureManager(UsageKeyFactoryMixin, ChildrenMapTestMixin, Test
def test_get_collected_structure_version(self):
self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)
BlockStructureBlockData.VERSION += 1
self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)
assert TestTransformer1.collect_call_count == 2
self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
assert TestTransformer1.collect_call_count == 1
def test_clear(self):
self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)

45
openedx/core/djangoapps/content/block_structure/tests/test_store.py
View File

@@ -4,11 +4,9 @@ Tests for block_structure/cache.py
import pytest
import ddt
from edx_toggles.toggles.testutils import override_waffle_switch
from openedx.core.djangolib.testing.utils import CacheIsolationTestCase
from ..config import STORAGE_BACKING_FOR_CACHE
from ..config.models import BlockStructureConfiguration
from ..exceptions import BlockStructureNotFound
from ..store import BlockStructureStore
@@ -46,40 +44,27 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
value=f'{transformer.name()} val',
)
@ddt.data(True, False)
def test_get_none(self, with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
with pytest.raises(BlockStructureNotFound):
self.store.get(self.block_structure.root_block_usage_key)
def test_get_none(self):
with pytest.raises(BlockStructureNotFound):
self.store.get(self.block_structure.root_block_usage_key)
@ddt.data(True, False)
def test_add_and_get(self, with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
self.store.add(self.block_structure)
stored_value = self.store.get(self.block_structure.root_block_usage_key)
assert stored_value is not None
self.assert_block_structure(stored_value, self.children_map)
@ddt.data(True, False)
def test_delete(self, with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
self.store.add(self.block_structure)
self.store.delete(self.block_structure.root_block_usage_key)
with pytest.raises(BlockStructureNotFound):
self.store.get(self.block_structure.root_block_usage_key)
def test_uncached_without_storage(self):
def test_add_and_get(self):
self.store.add(self.block_structure)
self.mock_cache.map.clear()
stored_value = self.store.get(self.block_structure.root_block_usage_key)
assert stored_value is not None
self.assert_block_structure(stored_value, self.children_map)
def test_delete(self):
self.store.add(self.block_structure)
self.store.delete(self.block_structure.root_block_usage_key)
with pytest.raises(BlockStructureNotFound):
self.store.get(self.block_structure.root_block_usage_key)
def test_uncached_with_storage(self):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=True):
self.store.add(self.block_structure)
self.mock_cache.map.clear()
stored_value = self.store.get(self.block_structure.root_block_usage_key)
self.assert_block_structure(stored_value, self.children_map)
self.store.add(self.block_structure)
self.mock_cache.map.clear()
stored_value = self.store.get(self.block_structure.root_block_usage_key)
self.assert_block_structure(stored_value, self.children_map)
@ddt.data(1, 5, None)
def test_cache_timeout(self, timeout):