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!: Drop the block_structure.storage_backing_for_cache WaffleSwitch #35185

Merged
merged 16 commits into from
Nov 6, 2024
Merged
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
6 changes: 5 additions & 1 deletion cms/djangoapps/contentstore/management/commands/import.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from openedx.core.djangoapps.django_comment_common.utils import are_permissions_roles_seeded, seed_permissions_roles
from xmodule.contentstore.django import contentstore # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.django import SignalHandler, modulestore # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.modulestore.xml_importer import import_course_from_xml # lint-amnesty, pylint: disable=wrong-import-order
from xmodule.util.sandboxing import DEFAULT_PYTHON_LIB_FILENAME # lint-amnesty, pylint: disable=wrong-import-order

Expand Down Expand Up @@ -73,6 +73,10 @@ def handle(self, *args, **options):

for course in course_items:
course_id = course.id
# Importing is an act of publishing so send the course published signal.
SignalHandler.course_published.send_robust(sender=self, course_key=course_id)
feanil marked this conversation as resolved.
Show resolved Hide resolved

# Seed forum permission roles if we need to.
if not are_permissions_roles_seeded(course_id):
self.stdout.write(f'Seeding forum roles for course {course_id}\n')
seed_permissions_roles(course_id)
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
__test__ = True

# TODO: decrease query count as part of REVO-28
QUERY_COUNT = 33
QUERY_COUNT = 34

TEST_DATA = {
('no_overrides', 1, True, False): (QUERY_COUNT, 2),
Expand Down
46 changes: 17 additions & 29 deletions lms/djangoapps/course_api/blocks/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,13 @@
"""


from itertools import product
from unittest.mock import patch

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 @@ -209,34 +206,25 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase):
Tests query counts for the get_blocks function.
"""

@ddt.data(
*product(
(ModuleStoreEnum.Type.split, ),
(True, False),
@ddt.data(ModuleStoreEnum.Type.split)
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.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,
)

@ddt.data(
(ModuleStoreEnum.Type.split, 2, True, 24),
(ModuleStoreEnum.Type.split, 2, False, 14),
(ModuleStoreEnum.Type.split, 2, 24),
)
@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,
)
def test_query_counts_uncached(self, store_type, expected_mongo_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,
)
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from lms.djangoapps.course_blocks.api import get_course_blocks
from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase
from lms.djangoapps.gating import api as lms_gating_api
import openedx.core.djangoapps.content.block_structure.api as bs_api
from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers
from openedx.core.djangoapps.course_apps.toggles import EXAMS_IDA
from openedx.core.lib.gating import api as gating_api
Expand Down Expand Up @@ -166,7 +167,11 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c
self.course.enable_subsection_gating = True
self.setup_gated_section(self.blocks[gated_block_ref], self.blocks[gating_block_ref])

with self.assertNumQueries(5):
# Cache the course blocks so that they don't need to be generated when we're trying to
# get data back. This would happen as a part of publishing in a production system.
bs_api.update_course_in_cache(self.course.id)

with self.assertNumQueries(4):
self.get_blocks_and_check_against_expected(self.user, expected_blocks_before_completion)

# clear the request cache to simulate a new request
Expand All @@ -179,7 +184,7 @@ def test_gated(self, gated_block_ref, gating_block_ref, expected_blocks_before_c
self.user,
)

with self.assertNumQueries(6):
with self.assertNumQueries(4):
self.get_blocks_and_check_against_expected(self.user, self.ALL_BLOCKS_EXCEPT_SPECIAL)

def test_staff_access(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache
from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers

import openedx.core.djangoapps.content.block_structure.api as bs_api
from ...api import get_course_blocks
from ..library_content import ContentLibraryOrderTransformer, ContentLibraryTransformer
from .helpers import CourseStructureTestCase
Expand Down Expand Up @@ -41,6 +42,8 @@ def setUp(self):
self.course_hierarchy = self.get_course_hierarchy()
self.blocks = self.build_course(self.course_hierarchy)
self.course = self.blocks['course']
# Do this manually because publish signals are not fired by default in tests.
bs_api.update_course_in_cache(self.course.id)
clear_course_from_cache(self.course.id)

# Enroll user in course.
Expand Down Expand Up @@ -122,6 +125,7 @@ def test_content_library(self):
)
assert len(list(raw_block_structure.get_block_keys())) == len(self.blocks)

bs_api.update_course_in_cache(self.course.id)
clear_course_from_cache(self.course.id)
trans_block_structure = get_course_blocks(
self.user,
Expand Down Expand Up @@ -175,6 +179,7 @@ def setUp(self):
self.course_hierarchy = self.get_course_hierarchy()
self.blocks = self.build_course(self.course_hierarchy)
self.course = self.blocks['course']
bs_api.update_course_in_cache(self.course.id)
clear_course_from_cache(self.course.id)

# Enroll user in course.
Expand Down
8 changes: 4 additions & 4 deletions lms/djangoapps/courseware/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1472,8 +1472,8 @@ def test_view_certificate_link(self):
self.assertContains(resp, "earned a certificate for this course.")

@ddt.data(
(True, 53),
(False, 53),
(True, 54),
(False, 54),
)
@ddt.unpack
def test_progress_queries_paced_courses(self, self_paced, query_count):
Expand All @@ -1488,13 +1488,13 @@ def test_progress_queries(self):
ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1))
self.setup_course()
with self.assertNumQueries(
53, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST
54, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST
), check_mongo_calls(2):
self._get_progress_page()

for _ in range(2):
with self.assertNumQueries(
38, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST
39, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST
), check_mongo_calls(2):
self._get_progress_page()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory

import openedx.core.djangoapps.content.block_structure.api as bs_api
from common.djangoapps.course_modes.models import CourseMode
from common.djangoapps.student.roles import (
CourseBetaTesterRole,
Expand Down Expand Up @@ -2228,6 +2229,9 @@ def test_get_override_for_unreleased_block(self):
display_name='Unreleased Section',
)

# We need to update the course in the cache after we create the new block.
bs_api.update_course_in_cache(self.course_data.course_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing it here manually is okay. I'd probably just enable the published signal for this test case and let it build everything–to test part of the machinery as well, since the triggers for that may change over time. It might involve splitting off this test from the rest of the test case though.

I wouldn't put it in xmodule/modulestore/tests/factories.py though, since that's called by a lot of things that may not require this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I think I'll leave it as is then.


resp = self.client.get(
self.get_url(subsection_id=unreleased_subsection.location)
)
Expand Down
66 changes: 34 additions & 32 deletions lms/djangoapps/grades/tests/integration/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from crum import set_current_request

import openedx.core.djangoapps.content.block_structure.api as bs_api
from xmodule.capa.tests.response_xml_factory import MultipleChoiceResponseXMLFactory
from common.djangoapps.student.models import CourseEnrollment
from common.djangoapps.student.tests.factories import UserFactory
Expand Down Expand Up @@ -76,44 +77,45 @@ def setUp(self):
CourseEnrollment.enroll(self.student, self.course.id)
self.instructor = UserFactory.create(is_staff=True, username='test_instructor', password=self.TEST_PASSWORD)
self.refresh_course()
# Since this doesn't happen automatically and we don't want to run all the publish signal handlers
# Just make sure we have the latest version of the course in cache before we test the problem.
bs_api.update_course_in_cache(self.course.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I'd probably emit the signal and let everything run (I think this is the most expensive part of it anyhow), but I think either way is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this class already has the course_published signal enabled and so I thought he issue might be that the CourseFactor needed the emit_signals parameter set to true, but even with that this did not work. How important is it that this be figured out for this PR? I can dig into it further if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work when you test the functionality locally via browser? If it doesn't happen automatically, it's possible that something is going wrong (e.g. it's going to trigger later on lazy-load). I'm really sorry to keep dragging this PR out, but so many things rely on block structures that I think it's worth investigating why it doesn't refresh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ormsbee I've tested it locally and created a new course and made various changes to the course sections/subsections and I can see them all propagate correctly to the course outline page. Let me know if there is anything else you want me to test.


@patch('lms.djangoapps.grades.events.tracker')
def test_submit_answer(self, events_tracker):
self.submit_question_answer('p1', {'2_1': 'choice_choice_2'})
course = self.store.get_course(self.course.id, depth=0)

event_transaction_id = events_tracker.emit.mock_calls[0][1][1]['event_transaction_id']
events_tracker.emit.assert_has_calls(
[
mock_call(
events.PROBLEM_SUBMITTED_EVENT_TYPE,
{
'user_id': str(self.student.id),
'event_transaction_id': event_transaction_id,
'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE,
'course_id': str(self.course.id),
'problem_id': str(self.problem.location),
'weighted_earned': 2.0,
'weighted_possible': 2.0,
},
),
mock_call(
events.COURSE_GRADE_CALCULATED,
{
'course_version': str(course.course_version),
'percent_grade': 0.02,
'grading_policy_hash': 'ChVp0lHGQGCevD0t4njna/C44zQ=',
'user_id': str(self.student.id),
'letter_grade': '',
'event_transaction_id': event_transaction_id,
'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE,
'course_id': str(self.course.id),
'course_edited_timestamp': str(course.subtree_edited_on),
}
),
],
any_order=True,
)
expected_calls = [
mock_call(
events.PROBLEM_SUBMITTED_EVENT_TYPE,
{
'user_id': str(self.student.id),
'event_transaction_id': event_transaction_id,
'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE,
'course_id': str(self.course.id),
'problem_id': str(self.problem.location),
'weighted_earned': 2.0,
'weighted_possible': 2.0,
},
),
mock_call(
events.COURSE_GRADE_CALCULATED,
{
'course_version': str(self.course.course_version),
'percent_grade': 0.02,
'grading_policy_hash': 'ChVp0lHGQGCevD0t4njna/C44zQ=',
'user_id': str(self.student.id),
'letter_grade': '',
'event_transaction_id': event_transaction_id,
'event_transaction_type': events.PROBLEM_SUBMITTED_EVENT_TYPE,
'course_id': str(self.course.id),
'course_edited_timestamp': str(self.course.subtree_edited_on),
}
),
]

events_tracker.emit.assert_has_calls(expected_calls, any_order=True)

@ddt.data(True, False)
def test_delete_student_state(self, emit_signals):
Expand Down
20 changes: 10 additions & 10 deletions lms/djangoapps/grades/tests/test_course_grade_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,35 +68,35 @@ def _assert_section_order(course_grade):
self.sequence2.display_name
]

with self.assertNumQueries(4), mock_get_score(1, 2):
with self.assertNumQueries(5), mock_get_score(1, 2):
_assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0

num_queries = 42
num_queries = 43
with self.assertNumQueries(num_queries), mock_get_score(1, 2):
grade_factory.update(self.request.user, self.course, force_update_subsections=True)

with self.assertNumQueries(3):
with self.assertNumQueries(4):
_assert_read(expected_pass=True, expected_percent=0.5) # updated to grade of .5

num_queries = 6
num_queries = 7
with self.assertNumQueries(num_queries), mock_get_score(1, 4):
grade_factory.update(self.request.user, self.course, force_update_subsections=False)

with self.assertNumQueries(3):
with self.assertNumQueries(4):
_assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25

num_queries = 18
num_queries = 19
with self.assertNumQueries(num_queries), mock_get_score(2, 2):
grade_factory.update(self.request.user, self.course, force_update_subsections=True)

with self.assertNumQueries(3):
with self.assertNumQueries(4):
_assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0

num_queries = 28
num_queries = 29
with self.assertNumQueries(num_queries), mock_get_score(0, 0): # the subsection now is worth zero
grade_factory.update(self.request.user, self.course, force_update_subsections=True)

with self.assertNumQueries(3):
with self.assertNumQueries(4):
_assert_read(expected_pass=False, expected_percent=0.0) # updated to grade of 0.0

@ddt.data((True, False))
Expand Down Expand Up @@ -286,7 +286,7 @@ def test_grading_exception(self, mock_course_grade):
else mock_course_grade.return_value
for student in self.students
]
with self.assertNumQueries(11):
with self.assertNumQueries(20):
all_course_grades, all_errors = self._course_grades_and_errors_for(self.course, self.students)
assert {student: str(all_errors[student]) for student in all_errors} == {
student3: 'Error for student3.',
Expand Down
8 changes: 4 additions & 4 deletions lms/djangoapps/grades/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@ def test_block_structure_created_only_once(self):
assert mock_block_structure_create.call_count == 1

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 41, True),
(ModuleStoreEnum.Type.split, 2, 41, False),
(ModuleStoreEnum.Type.split, 2, 42, True),
(ModuleStoreEnum.Type.split, 2, 42, False),
)
@ddt.unpack
def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections):
Expand All @@ -168,7 +168,7 @@ def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, creat
self._apply_recalculate_subsection_grade()

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 41),
(ModuleStoreEnum.Type.split, 2, 42),
)
@ddt.unpack
def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls):
Expand Down Expand Up @@ -255,7 +255,7 @@ def test_problem_block_with_restricted_access(self, mock_subsection_signal):
UserPartition.scheme_extensions = None

@ddt.data(
(ModuleStoreEnum.Type.split, 2, 41),
(ModuleStoreEnum.Type.split, 2, 42),
)
@ddt.unpack
def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries):
Expand Down
2 changes: 2 additions & 0 deletions lms/djangoapps/grades/tests/test_transformer.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
from xmodule.modulestore.tests.factories import check_mongo_calls_range

import openedx.core.djangoapps.content.block_structure.api as bs_api
from common.djangoapps.student.tests.factories import UserFactory
from lms.djangoapps.course_blocks.api import get_course_blocks
from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase
Expand Down Expand Up @@ -462,6 +463,7 @@ def test_modulestore_performance(self, store_type, max_mongo_calls, min_mongo_ca
)
with self.store.default_store(store_type):
blocks = self.build_course(course)
bs_api.update_course_in_cache(blocks['course'].id)
clear_course_from_cache(blocks['course'].id)
with check_mongo_calls_range(max_mongo_calls, min_mongo_calls):
get_course_blocks(self.student, blocks['course'].location, self.transformers)
Loading
Loading