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 #33108

Closed
wants to merge 38 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0d88c01
feat: block_structure.storage_backing_for_cache toggle removed depr32
Aug 28, 2023
6775629
feat: update test_dump_to_neo4j.py
Aug 28, 2023
7e20bad
feat: update test_api.py
Aug 28, 2023
fd29299
feat: update store.py
Aug 28, 2023
ca3add7
feat: update common.py
Aug 28, 2023
8e3c43a
feat: update test_api.py
Aug 28, 2023
18ec48c
feat: Update common.py
Yagnesh1998 Aug 29, 2023
4b56614
feat!: upgrading django-storages to 1.10.1 (#32571)
awais786 Aug 28, 2023
b744397
fix: fix django 41 deprecation warnings (#33099)
UsamaSadiq Aug 28, 2023
9d04bd0
feat: add hook to modify courserun data for executive education courses
jajjibhai008 Aug 18, 2023
4054f7d
Revert "feat!: upgrading django-storages to 1.10.1 (#32571)" (#33109)
awais786 Aug 28, 2023
a87dafc
Revert "Revert "feat!: upgrading django-storages to 1.10.1 (#32571)" …
awais786 Aug 28, 2023
a2bedeb
feat: adding unenrollments to event bus (#33085)
kiram15 Aug 28, 2023
04b01c2
fix: incorrect type hints in a few places (#33104)
bradenmacdonald Aug 28, 2023
91752aa
chore: version bump
katrinan029 Aug 29, 2023
89034db
feat: update test_api.py
Aug 29, 2023
43c4876
feat!: upgrading `django-storages` to `1.11.1` (#33115)
awais786 Aug 29, 2023
3b15fd6
fix: func.__name__ issue (#33133)
irtazaakram Aug 30, 2023
89129e8
fix: object has no attribute is_ajax (#33134)
irtazaakram Aug 30, 2023
c759b5c
chore: Updating Python Requirements (#33128)
edx-requirements-bot Aug 30, 2023
8f57194
Revert "Revert "Revert "feat!: upgrading django-storages to 1.10.1 (#…
awais786 Aug 30, 2023
1b4cf50
feat: release edx-enterprise 4.1.4 (#33135)
johnnagro Aug 30, 2023
981936f
feat: add API endpoint for asset usage search (#33092)
KristinAoki Aug 30, 2023
19c8fcd
feat: Upgrade Python dependency edx-enterprise
alex-sheehan-edx Aug 30, 2023
9115235
chore: geoip2: update maxmind geolite country database
Aug 30, 2023
e1aa213
feat: update generate_course_blocks.py
Aug 31, 2023
dd1f6e1
feat: update test_generate_course_blocks.py
Aug 31, 2023
fff7cc8
feat!: upgrading django-storages to 1.11.1
awais786 Aug 31, 2023
9f31a53
feat: added new_question_post and new_discussion_post notification (#…
muhammadadeeltajamul Aug 31, 2023
8aed470
feat: make notification type info translateable (#33071)
muhammadadeeltajamul Aug 31, 2023
dce44e5
fix: handle new_comment notification edge case (#33088)
muhammadadeeltajamul Aug 31, 2023
8a03502
test: adding check to run migrations tests only if its django32. (#33…
awais786 Aug 31, 2023
3f5143e
fix: better username lookup in tpa pipeline (#33145)
johnnagro Aug 31, 2023
612ba7c
build: don't exclude lms/static/css/vendor from Docker build (#33143)
kdmccormick Aug 31, 2023
52dbd01
fix: bump edx-enterprise (#33149)
kiram15 Aug 31, 2023
d0ae267
feat: update lms_common.py
Sep 1, 2023
6c65821
feat update lms_common.py
Sep 1, 2023
fd4224e
Merge branch 'master' into edx-depr32
Yagnesh1998 Sep 1, 2023
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 @@ -9,11 +9,9 @@
import ddt
from django.core.management import call_command
from django.test.utils import override_settings
from edx_toggles.toggles.testutils import override_waffle_switch
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 +539,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
8 changes: 8 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@

# Maximum number of retries per task.
TASK_MAX_RETRIES=5,

#block structures are stored in a more permanent storage
#which provides an additional backup for cache misses
STORAGE_CLASS='django.core.files.storage.FileSystemStorage',
STORAGE_KWARGS=dict(
location=MEDIA_ROOT,
base_url=MEDIA_URL,
),
)

############################ FEATURE CONFIGURATION #############################
Expand Down
35 changes: 14 additions & 21 deletions lms/djangoapps/course_api/blocks/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@

import ddt
from django.test.client import RequestFactory
from edx_toggles.toggles.testutils import override_waffle_switch

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 @@ -212,31 +210,26 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
@ddt.data(
*product(
(ModuleStoreEnum.Type.split, ),
(True, False),
)
)
@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,
)
def test_query_counts_cached(self, store_type):
course = self._create_course(store_type)
self._get_blocks(
course,
expected_mongo_queries=0,
expected_sql_queries=14,
)

@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,
)
8 changes: 8 additions & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2885,6 +2885,14 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
# For more information, check https://github.com/openedx/edx-platform/pull/13388 and
# https://github.com/openedx/edx-platform/pull/14571.
TASK_MAX_RETRIES=5,

#block structures are stored in a more permanent storage
#which provides an additional backup for cache misses
STORAGE_CLASS='django.core.files.storage.FileSystemStorage',
STORAGE_KWARGS=dict(
location=MEDIA_ROOT,
base_url=MEDIA_URL,
),
)

################################ Bulk Email ###################################
Expand Down
31 changes: 0 additions & 31 deletions openedx/core/djangoapps/content/block_structure/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,6 @@

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():
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 @@ -75,12 +74,6 @@ def add_arguments(self, parser):
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):

Expand Down Expand Up @@ -129,9 +122,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:
self._generate_for_course(options, course_key)
Expand All @@ -150,7 +140,7 @@ def _generate_for_course(self, options, course_key):
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@
import ddt
from django.core.management.base import CommandError

from openedx.core.djangoapps.content.block_structure.tests.helpers import (
is_course_in_block_structure_cache,
is_course_in_block_structure_storage
)
from openedx.core.djangoapps.content.block_structure.tests.helpers import is_course_in_block_structure_storage
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order

Expand All @@ -35,20 +32,6 @@ def setUp(self):
self.course_keys = [course.id for course in self.courses]
self.command = generate_course_blocks.Command()

def _assert_courses_not_in_block_cache(self, *course_keys):
"""
Assert courses don't exist in the course block cache.
"""
for course_key in course_keys:
assert not is_course_in_block_structure_cache(course_key, self.store)

def _assert_courses_in_block_cache(self, *course_keys):
"""
Assert courses exist in course block cache.
"""
for course_key in course_keys:
assert is_course_in_block_structure_cache(course_key, self.store)

def _assert_courses_not_in_block_storage(self, *course_keys):
"""
Assert courses don't exist in course block storage.
Expand All @@ -75,25 +58,19 @@ def _assert_message_presence_in_logs(self, message, mock_log, expected_presence=

@ddt.data(True, False)
def test_all_courses(self, force_update):
self._assert_courses_not_in_block_cache(*self.course_keys)
self.command.handle(all_courses=True)
self._assert_courses_in_block_cache(*self.course_keys)
with patch(
'openedx.core.djangoapps.content.block_structure.factory.BlockStructureFactory.create_from_modulestore'
) as mock_update_from_store:
self.command.handle(all_courses=True, force_update=force_update)
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.command.handle(courses=[str(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:])

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
51 changes: 16 additions & 35 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 @@ -183,12 +174,7 @@ def _get_from_store(self, bs_model):
"""
Returns the serialized data for the given BlockStructureModel
from storage.
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 @@ -226,14 +212,9 @@ def _deserialize(self, serialized_data, root_block_usage_key):
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):
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
Loading