Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: block_structure.storage_backing_for_cache toggle removed depr32 #33090

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory

import openedx.core.djangoapps.content.block_structure.config as block_structure_config
from openedx.core.djangoapps.content.block_structure.signals import update_block_structure_on_course_publish
from cms.djangoapps.coursegraph.management.commands.dump_to_neo4j import ModuleStoreSerializer
from cms.djangoapps.coursegraph.management.commands.tests.utils import MockGraph, MockNodeMatcher
Expand Down Expand Up @@ -541,8 +540,7 @@ def test_dump_to_neo4j_published(self, mock_graph_constructor, mock_matcher_clas
assert len(submitted) == len(self.course_strings)

# simulate one of the courses being published
with override_waffle_switch(block_structure_config.STORAGE_BACKING_FOR_CACHE, True):
update_block_structure_on_course_publish(None, self.course.id)
update_block_structure_on_course_publish(None, self.course.id)

# make sure only the published course was dumped
submitted, __ = self.mss.dump_courses_to_neo4j(credentials)
Expand Down
5 changes: 5 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@

# Maximum number of retries per task.
TASK_MAX_RETRIES=5,
STORAGE_CLASS='django.core.files.storage.FileSystemStorage',
STORAGE_KWARGS=dict(
location=MEDIA_ROOT,
base_url=MEDIA_URL,
),
)

############################ FEATURE CONFIGURATION #############################
Expand Down
31 changes: 14 additions & 17 deletions lms/djangoapps/course_api/blocks/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

from common.djangoapps.student.tests.factories import UserFactory
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
from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import SampleCourseFactory, check_mongo_calls # lint-amnesty, pylint: disable=wrong-import-order
Expand Down Expand Up @@ -217,26 +216,24 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
)
@ddt.unpack
def test_query_counts_cached(self, store_type, 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,
expected_mongo_queries=0,
expected_sql_queries=14 if with_storage_backing else 13,
)
course = self._create_course(store_type)
self._get_blocks(
course,
expected_mongo_queries=0,
expected_sql_queries=14 if with_storage_backing else 13,
)

@ddt.data(
(ModuleStoreEnum.Type.split, 2, True, 23),
(ModuleStoreEnum.Type.split, 2, False, 13),
)
@ddt.unpack
def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries):
with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
course = self._create_course(store_type)
clear_course_from_cache(course.id)

self._get_blocks(
course,
expected_mongo_queries,
expected_sql_queries=num_sql_queries,
)
course = self._create_course(store_type)
clear_course_from_cache(course.id)

self._get_blocks(
course,
expected_mongo_queries,
expected_sql_queries=num_sql_queries,
)
12 changes: 0 additions & 12 deletions openedx/core/djangoapps/content/block_structure/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,6 @@
# .. 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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
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,
Expand Down Expand Up @@ -129,8 +128,6 @@ def _generate_course_blocks(self, options, course_keys):
"""
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:
Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/content/block_structure/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ def get(cls, data_usage_key):
"""
try:
return cls.objects.get(data_usage_key=data_usage_key)
except cls.DoesNotExist:
except:
log.info('BlockStructure: Not found in table; %s.', data_usage_key)
raise BlockStructureNotFound(data_usage_key) # lint-amnesty, pylint: disable=raise-missing-from

Expand Down
47 changes: 15 additions & 32 deletions openedx/core/djangoapps/content/block_structure/store.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,40 +120,31 @@ def is_up_to_date(self, root_block_usage_key, modulestore):
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

return False
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

def _get_model(self, root_block_usage_key):
"""
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):
"""
Expand Down Expand Up @@ -186,9 +177,6 @@ def _get_from_store(self, bs_model):
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):
Expand Down Expand Up @@ -228,12 +216,7 @@ def _encode_root_cache_key(bs_model):
Returns the cache key to use for the given
BlockStructureModel or StubModel.
"""
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):
Expand Down
5 changes: 0 additions & 5 deletions openedx/core/djangoapps/content/block_structure/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

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')
Expand Down Expand Up @@ -62,8 +61,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)


Expand Down Expand Up @@ -93,8 +90,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)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,6 @@ def test_from_modulestore_fail(self):
modulestore=self.modulestore,
)

def test_from_cache(self):
store = BlockStructureStore(MockCache())
block_structure = self.create_block_structure(self.children_map)
store.add(block_structure)
from_cache_block_structure = BlockStructureFactory.create_from_store(
block_structure.root_block_usage_key,
store,
)
self.assert_block_structure(from_cache_block_structure, self.children_map)

def test_from_cache_none(self):
store = BlockStructureStore(MockCache())
with pytest.raises(BlockStructureNotFound):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@
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
Expand Down Expand Up @@ -177,21 +174,6 @@ def test_get_collected_cached(self):
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

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.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)

Expand All @@ -212,8 +194,8 @@ def test_get_collected_transformer_version(self):
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,10 @@
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
from .helpers import ChildrenMapTestMixin, MockCache, MockTransformer, UsageKeyFactoryMixin

Expand Down Expand Up @@ -46,40 +41,11 @@ def add_transformers(self):
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)

@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_uncached_with_storage(self):
self.store.add(self.block_structure)
self.mock_cache.map.clear()
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)
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):
Expand Down
Loading