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

Get rid of calls to set_request_cache_with_short_name

Browse Source 
This method from the toggle legacy classes should not actually be
exposed to all. So we get rid of it by manually setting the cached
value. While we are at it, we convert the STORAGE_BACKING_FOR_CACHE
legacy waffle switch to its modern version. As the flag is not being
used elsewhere, this should not break anything.

We take the opportunity to modernize waffle switches from
block_structure.config: to do so we convert the INVALIDATE_CACHE_ON_PUBLISH and
RAISE_ERROR_WHEN_NOT_FOUND waffle switches from legacy classes to their modern
equivalents. These switches are not used outside of edx-platform, so this
change should not trigger any error.
This commit is contained in:
Régis Behmo
2021-01-08 18:32:29 +01:00
parent 177c1a530f
commit 3a29cff016
11 changed files with 50 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
lms/djangoapps/course_api/blocks/tests/test_api.py
View File

@@ -17,7 +17,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls
from xmodule.modulestore.tests.sample_courses import BlockInfo
from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache
from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle_switch
from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE
from ..api import get_blocks
@@ -230,7 +230,7 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
)
@ddt.unpack
def test_query_counts_cached(self, store_type, with_storage_backing):
with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
course = self._create_course(store_type)
self._get_blocks(
course,
@@ -247,7 +247,7 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
@ddt.unpack
def test_query_counts_uncached(self, store_type_tuple, with_storage_backing):
store_type, expected_mongo_queries = store_type_tuple
with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
course = self._create_course(store_type)
clear_course_from_cache(course.id)

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

@@ -2,37 +2,32 @@
This module contains various configuration settings via
waffle switches for the Block Structure framework.
"""
from edx_django_utils.cache import RequestCache
from edx_toggles.toggles.__future__ import WaffleSwitch
from edx_toggles.toggles import LegacyWaffleSwitch, LegacyWaffleSwitchNamespace
from openedx.core.lib.cache_utils import request_cached
from .models import BlockStructureConfiguration
# Namespace
WAFFLE_NAMESPACE = u'block_structure'
# Switches
INVALIDATE_CACHE_ON_PUBLISH = u'invalidate_cache_on_publish'
STORAGE_BACKING_FOR_CACHE = u'storage_backing_for_cache'
RAISE_ERROR_WHEN_NOT_FOUND = u'raise_error_when_not_found'
INVALIDATE_CACHE_ON_PUBLISH = WaffleSwitch(
"block_structure.invalidate_cache_on_publish", __name__
)
STORAGE_BACKING_FOR_CACHE = WaffleSwitch(
"block_structure.storage_backing_for_cache", __name__
)
RAISE_ERROR_WHEN_NOT_FOUND = WaffleSwitch(
"block_structure.raise_error_when_not_found", __name__
)
def waffle():
def enable_storage_backing_for_cache_in_request():
"""
Returns the namespaced and cached Waffle class for BlockStructures.
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.
"""
return LegacyWaffleSwitchNamespace(name=WAFFLE_NAMESPACE, log_prefix=u'BlockStructure: ')
def waffle_switch(name):
"""
Return the waffle switch associated to this namespace.
WARNING: do not replicate this pattern. Instead of declaring waffle switch names as strings, you should create
LegacyWaffleSwitch objects as top-level constants.
"""
return LegacyWaffleSwitch(waffle(), name, module_name=__name__)
# pylint: disable=protected-access
STORAGE_BACKING_FOR_CACHE._cached_switches[STORAGE_BACKING_FOR_CACHE.name] = True
@request_cached()

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

@@ -7,13 +7,12 @@ import logging
import six
from django.core.management.base import BaseCommand
import six
from six import text_type
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 STORAGE_BACKING_FOR_CACHE, waffle
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,
@@ -133,7 +132,7 @@ class Command(BaseCommand):
Generates course blocks for the given course_keys per the given options.
"""
if options.get('with_storage'):
waffle().set_request_cache_with_short_name(STORAGE_BACKING_FOR_CACHE, True)
enable_storage_backing_for_cache_in_request()
for course_key in course_keys:
try:

5
openedx/core/djangoapps/content/block_structure/manager.py
View File

@@ -102,10 +102,9 @@ class BlockStructureManager(object):
BlockStructureTransformers.verify_versions(block_structure)
except (BlockStructureNotFound, TransformerDataIncompatible):
if config.waffle().is_enabled(config.RAISE_ERROR_WHEN_NOT_FOUND):
if config.RAISE_ERROR_WHEN_NOT_FOUND.is_enabled():
raise
else:
block_structure = self._update_collected()
block_structure = self._update_collected()
return block_structure

2
openedx/core/djangoapps/content/block_structure/signals.py
View File

@@ -29,7 +29,7 @@ def update_block_structure_on_course_publish(sender, course_key, **kwargs): # p
if isinstance(course_key, LibraryLocator):
return
if config.waffle().is_enabled(config.INVALIDATE_CACHE_ON_PUBLISH):
if config.INVALIDATE_CACHE_ON_PUBLISH.is_enabled():
try:
clear_course_from_cache(course_key)
except BlockStructureNotFound:

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

@@ -7,8 +7,8 @@ Module for the Storage of BlockStructure objects.
from logging import getLogger
import six
from django.utils.encoding import python_2_unicode_compatible
from openedx.core.lib.cache_utils import zpickle, zunpickle
from . import config
@@ -28,6 +28,7 @@ class StubModel(object):
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
@@ -45,6 +46,7 @@ class BlockStructureStore(object):
"""
Storage for BlockStructure objects.
"""
def __init__(self, cache):
"""
Arguments:
@@ -121,7 +123,7 @@ class BlockStructureStore(object):
Returns whether the data in storage for the given key is
already up-to-date with the version in the given modulestore.
"""
if _is_storage_backing_enabled():
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)
@@ -135,7 +137,7 @@ class BlockStructureStore(object):
"""
Returns the model associated with the given key.
"""
if _is_storage_backing_enabled():
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
return BlockStructureModel.get(root_block_usage_key)
else:
return StubModel(root_block_usage_key)
@@ -145,7 +147,7 @@ class BlockStructureStore(object):
Updates or creates the model for the given block_structure
and serialized_data.
"""
if _is_storage_backing_enabled():
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,
@@ -187,7 +189,7 @@ class BlockStructureStore(object):
Raises:
BlockStructureNotFound if not found.
"""
if not _is_storage_backing_enabled():
if not config.STORAGE_BACKING_FOR_CACHE.is_enabled():
raise BlockStructureNotFound(bs_model.data_usage_key)
return bs_model.get_serialized_data()
@@ -229,14 +231,12 @@ class BlockStructureStore(object):
Returns the cache key to use for the given
BlockStructureModel or StubModel.
"""
if _is_storage_backing_enabled():
if config.STORAGE_BACKING_FOR_CACHE.is_enabled():
return six.text_type(bs_model)
else:
return "v{version}.root.key.{root_usage_key}".format(
version=six.text_type(BlockStructureBlockData.VERSION),
root_usage_key=six.text_type(bs_model.data_usage_key),
)
return "v{version}.root.key.{root_usage_key}".format(
version=six.text_type(BlockStructureBlockData.VERSION),
root_usage_key=six.text_type(bs_model.data_usage_key),
)
@staticmethod
def _version_data_of_block(root_block):
@@ -260,10 +260,3 @@ class BlockStructureStore(object):
field_name: getattr(bs_model, field_name, None)
for field_name in BlockStructureModel.VERSION_FIELDS
}
def _is_storage_backing_enabled():
"""
Returns whether storage backing for Block Structures is enabled.
"""
return config.waffle().is_enabled(config.STORAGE_BACKING_FOR_CACHE)

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

@@ -14,7 +14,7 @@ from opaque_keys.edx.keys import CourseKey
from capa.responsetypes import LoncapaProblemError
from openedx.core.djangoapps.content.block_structure import api
from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle
from openedx.core.djangoapps.content.block_structure.config import enable_storage_backing_for_cache_in_request
from xmodule.modulestore.exceptions import ItemNotFoundError
log = logging.getLogger('edx.celery.task')
@@ -63,7 +63,7 @@ def _update_course_in_cache(self, **kwargs):
Updates the course blocks (mongo -> BlockStructure) for the specified course.
"""
if kwargs.get('with_storage'):
waffle().set_request_cache_with_short_name(STORAGE_BACKING_FOR_CACHE, True)
enable_storage_backing_for_cache_in_request()
_call_and_retry_if_needed(self, api.update_course_in_cache, **kwargs)
@@ -94,7 +94,7 @@ 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'):
waffle().set_request_cache_with_short_name(STORAGE_BACKING_FOR_CACHE, True)
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/test_manager.py
View File

@@ -9,7 +9,7 @@ from django.test import TestCase
from edx_toggles.toggles.testutils import override_waffle_switch
from ..block_structure import BlockStructureBlockData
from ..config import RAISE_ERROR_WHEN_NOT_FOUND, STORAGE_BACKING_FOR_CACHE, waffle_switch
from ..config import RAISE_ERROR_WHEN_NOT_FOUND, STORAGE_BACKING_FOR_CACHE
from ..exceptions import BlockStructureNotFound, UsageKeyNotInBlockStructure
from ..manager import BlockStructureManager
from ..transformers import BlockStructureTransformers
@@ -179,14 +179,14 @@ class TestBlockStructureManager(UsageKeyFactoryMixin, ChildrenMapTestMixin, Test
assert TestTransformer1.collect_call_count == 1
def test_get_collected_error_raised(self):
with override_waffle_switch(waffle_switch(RAISE_ERROR_WHEN_NOT_FOUND), active=True):
with override_waffle_switch(RAISE_ERROR_WHEN_NOT_FOUND, active=True):
with mock_registered_transformers(self.registered_transformers):
with self.assertRaises(BlockStructureNotFound):
self.bs_manager.get_collected()
@ddt.data(True, False)
def test_update_collected_if_needed(self, with_storage_backing):
with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=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

4
openedx/core/djangoapps/content/block_structure/tests/test_signals.py
View File

@@ -12,7 +12,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory
from ..api import get_block_structure_manager
from ..config import INVALIDATE_CACHE_ON_PUBLISH, waffle_switch
from ..config import INVALIDATE_CACHE_ON_PUBLISH
from ..signals import update_block_structure_on_course_publish
from .helpers import is_course_in_block_structure_cache
@@ -56,7 +56,7 @@ class CourseBlocksSignalTest(ModuleStoreTestCase):
def test_cache_invalidation(self, invalidate_cache_enabled, mock_bs_manager_clear):
test_display_name = "Jedi 101"
with override_waffle_switch(waffle_switch(INVALIDATE_CACHE_ON_PUBLISH), active=invalidate_cache_enabled):
with override_waffle_switch(INVALIDATE_CACHE_ON_PUBLISH, active=invalidate_cache_enabled):
self.course.display_name = test_display_name
self.store.update_item(self.course, self.user.id)

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

@@ -8,7 +8,7 @@ from edx_toggles.toggles.testutils import override_waffle_switch
from openedx.core.djangolib.testing.utils import CacheIsolationTestCase
from ..config import STORAGE_BACKING_FOR_CACHE, waffle_switch
from ..config import STORAGE_BACKING_FOR_CACHE
from ..config.models import BlockStructureConfiguration
from ..exceptions import BlockStructureNotFound
from ..store import BlockStructureStore
@@ -48,13 +48,13 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
@ddt.data(True, False)
def test_get_none(self, with_storage_backing):
with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=with_storage_backing):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
with self.assertRaises(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(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=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)
self.assertIsNotNone(stored_value)
@@ -62,7 +62,7 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
@ddt.data(True, False)
def test_delete(self, with_storage_backing):
with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=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 self.assertRaises(BlockStructureNotFound):
@@ -75,7 +75,7 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
self.store.get(self.block_structure.root_block_usage_key)
def test_uncached_with_storage(self):
with override_waffle_switch(waffle_switch(STORAGE_BACKING_FOR_CACHE), active=True):
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)

4
openedx/core/djangoapps/coursegraph/management/commands/tests/test_dump_to_neo4j.py
View File

@@ -500,9 +500,7 @@ class TestModuleStoreSerializer(TestDumpToNeo4jCommandBase):
self.assertEqual(len(submitted), len(self.course_strings))
# simulate one of the courses being published
with override_waffle_switch(
block_structure_config.waffle_switch(block_structure_config.STORAGE_BACKING_FOR_CACHE), True
):
with override_waffle_switch(block_structure_config.STORAGE_BACKING_FOR_CACHE, True):
update_block_structure_on_course_publish(None, self.course.id)
# make sure only the published course was dumped