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.

Sign up for GitHub

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

fix: don't call signal handlers like XBLOCK_UPDATED before commit #34847

Merged
merged 2 commits into from
May 23, 2024
Merged

fix: don't call signal handlers like XBLOCK_UPDATED before commit #34847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6316ad2
fix: don't call signal handlers like XBLOCK_UPDATED before commit (#3…
bradenmacdonald May 22, 2024
723b779
docs: fix minor typos in the meilisearch feature configuration (#34838)
bradenmacdonald May 22, 2024
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
4 changes: 2 additions & 2 deletions cms/envs/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2953,13 +2953,13 @@ def _should_send_xblock_events(settings):

BEAMER_PRODUCT_ID = ""

################### Studio Search (alpha, using Meilisearch) ###################
################### Studio Search (beta), using Meilisearch ###################

# Enable Studio search features (powered by Meilisearch) (beta, off by default)
MEILISEARCH_ENABLED = False
# Meilisearch URL that the python backend can use. Often points to another docker container or k8s service.
MEILISEARCH_URL = "http://meilisearch"
# URL that browsers (end users) can user to reach Meilisearch. Should be HTTPS in production.
# URL that browsers (end users) can use to reach Meilisearch. Should be HTTPS in production.
MEILISEARCH_PUBLIC_URL = "http://meilisearch.example.com"
# To support multi-tenancy, you can prefix all indexes with a common key like "sandbox7-"
# and use a restricted tenant token in place of an API key, so that this Open edX instance
Expand Down
40 changes: 37 additions & 3 deletions xmodule/modulestore/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ def __init__(self):
self._active_count = 0
self.has_publish_item = False
self.has_library_updated_item = False
self._commit_callbacks = []

@property
def active(self):
Expand Down Expand Up @@ -148,6 +149,20 @@ def is_root(self):
"""
return self._active_count == 1

def defer_until_commit(self, fn):
"""
Run some code when the changes from this bulk op are committed to the DB
"""
self._commit_callbacks.append(fn)

def call_commit_callbacks(self):
"""
When the changes have been committed to the DB, call this to run any queued callbacks
"""
for fn in self._commit_callbacks:
fn()
self._commit_callbacks.clear()


class ActiveBulkThread(threading.local):
"""
Expand Down Expand Up @@ -290,15 +305,34 @@ def _end_bulk_operation(self, structure_key, emit_signals=True, ignore_case=Fals
# So re-nest until the signals are sent.
bulk_ops_record.nest()

if emit_signals and dirty:
self.send_bulk_published_signal(bulk_ops_record, structure_key)
self.send_bulk_library_updated_signal(bulk_ops_record, structure_key)
if dirty:
# Call any "on commit" callback, regardless of if this was "published" or is still draft:
bulk_ops_record.call_commit_callbacks()
# Call any "on publish" handlers - emit_signals is usually false for draft-only changes:
if emit_signals:
self.send_bulk_published_signal(bulk_ops_record, structure_key)
self.send_bulk_library_updated_signal(bulk_ops_record, structure_key)

# Signals are sent. Now unnest and clear the bulk op for good.
bulk_ops_record.unnest()

self._clear_bulk_ops_record(structure_key)

def on_commit_changes_to(self, course_key, fn):
"""
Call some callback when the currently active bulk operation has saved
"""
# Check if a bulk op is active. If so, defer fn(); otherwise call it immediately.
# Note: calling _get_bulk_ops_record() here and then checking .active can have side-effects in some cases
# because it creates an entry in the defaultdict if none exists, so we check if the record is active using
# the same code as _clear_bulk_ops_record(), which doesn't modify the defaultdict.
# so we check it this way:
if course_key and course_key.for_branch(None) in self._active_bulk_ops.records:
bulk_ops_record = self._active_bulk_ops.records[course_key.for_branch(None)]
bulk_ops_record.defer_until_commit(fn)
else:
fn() # There is no active bulk operation - call fn() now.

def _is_in_bulk_operation(self, course_key, ignore_case=False):
"""
Return whether a bulk operation is active on `course_key`.
Expand Down
89 changes: 54 additions & 35 deletions xmodule/modulestore/mixed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,15 +758,19 @@ def create_item(self, user_id, course_key, block_type, block_id=None, fields=Non
"""
modulestore = self._verify_modulestore_support(course_key, 'create_item')
xblock = modulestore.create_item(user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs)
# .. event_implemented_name: XBLOCK_CREATED
XBLOCK_CREATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=xblock.location.for_branch(None),
block_type=block_type,
version=xblock.location

def send_created_event():
# .. event_implemented_name: XBLOCK_CREATED
XBLOCK_CREATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=xblock.location.for_branch(None),
block_type=block_type,
version=xblock.location
)
)
)

modulestore.on_commit_changes_to(course_key, send_created_event)
return xblock

@strip_key
Expand All @@ -787,19 +791,24 @@ def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fie
fields (dict): A dictionary specifying initial values for some or all fields
in the newly created block
"""
modulestore = self._verify_modulestore_support(parent_usage_key.course_key, 'create_child')
xblock = modulestore.create_child(
course_key = parent_usage_key.course_key
store = self._verify_modulestore_support(course_key, 'create_child')
xblock = store.create_child(
user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs
)
# .. event_implemented_name: XBLOCK_CREATED
XBLOCK_CREATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=xblock.location.for_branch(None),
block_type=block_type,
version=xblock.location

def send_created_event():
# .. event_implemented_name: XBLOCK_CREATED
XBLOCK_CREATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=xblock.location.for_branch(None),
block_type=block_type,
version=xblock.location
)
)
)

store.on_commit_changes_to(course_key, send_created_event)
return xblock

@strip_key
Expand Down Expand Up @@ -828,34 +837,44 @@ def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): # lint
Update the xblock persisted to be the same as the given for all types of fields
(content, children, and metadata) attribute the change to the given user.
"""
store = self._verify_modulestore_support(xblock.location.course_key, 'update_item')
course_key = xblock.location.course_key
store = self._verify_modulestore_support(course_key, 'update_item')
xblock = store.update_item(xblock, user_id, allow_not_found, **kwargs)
# .. event_implemented_name: XBLOCK_UPDATED
XBLOCK_UPDATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=xblock.location.for_branch(None),
block_type=xblock.location.block_type,
version=xblock.location

def send_updated_event():
# .. event_implemented_name: XBLOCK_UPDATED
XBLOCK_UPDATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=xblock.location.for_branch(None),
block_type=xblock.location.block_type,
version=xblock.location
)
)
)

store.on_commit_changes_to(course_key, send_updated_event)
return xblock

@strip_key
def delete_item(self, location, user_id, **kwargs): # lint-amnesty, pylint: disable=arguments-differ
"""
Delete the given item from persistence. kwargs allow modulestore specific parameters.
"""
store = self._verify_modulestore_support(location.course_key, 'delete_item')
course_key = location.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.

@bradenmacdonald I had issues with deleting content from my course -- deleted blocks still show up in search?

But I'm not sure if there's something wrong with my setup, because I've got redis errors in the traceback.

cms-1  | 2024-05-22 23:47:11,093 ERROR 327 [django.dispatch] [user 6] [ip 172.18.0.1] dispatcher.py:213 - Error calling handle_item_deleted in Signal.send_robust() (BlockKey(type='vertical', id='f69a018879e5421f93fc631e8edd15da'))
cms-1  | Traceback (most recent call last):
cms-1  |   File "/openedx/venv/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 211, in send_robust
cms-1  |     response = receiver(signal=self, sender=sender, **named)
cms-1  |   File "/openedx/edx-platform/cms/djangoapps/contentstore/signals/handlers.py", line 207, in handle_item_deleted
cms-1  |     deleted_block = modulestore().get_item(usage_key)
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/mixed.py", line 90, in inner
cms-1  |     retval = func(field_decorator=strip_key_collection, *args, **kwargs)
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/mixed.py", line 257, in get_item
cms-1  |     return store.get_item(usage_key, depth, **kwargs)
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/split_mongo/split_draft.py", line 283, in get_item
cms-1  |     return super().get_item(usage_key, depth=depth, **kwargs)
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/split_mongo/split.py", line 1183, in get_item
cms-1  |     items = self._load_items(course, [BlockKey.from_usage_key(usage_key)], depth, **kwargs)
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/split_mongo/split.py", line 780, in _load_items
cms-1  |     return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/split_mongo/split.py", line 780, in <listcomp>
cms-1  |     return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 127, in _load_item
cms-1  |     block_data = self.get_module_data(block_key, course_key)
cms-1  |   File "/openedx/edx-platform/xmodule/modulestore/split_mongo/caching_descriptor_system.py", line 154, in get_module_data
cms-1  |     raise ItemNotFoundError(block_key)
cms-1  | xmodule.modulestore.exceptions.ItemNotFoundError: BlockKey(type='vertical', id='f69a018879e5421f93fc631e8edd15da')
cms-1  | 2024-05-22 23:47:11,094 INFO 327 [xmodule.modulestore.django] [user 6] [ip 172.18.0.1] django.py:220 - Sent item_deleted signal to <function handle_item_deleted at 0x7f831eca1e50> with kwargs {'usage_key': BlockUsageLocator(CourseLocator('SampleTaxonomyOrg2', 'STC1', '2023_1', 'draft-branch', None), 'vertical', 'f69a018879e5421f93fc631e8edd15da'), 'user_id': 6}. Response was: BlockKey(type='vertical', id='f69a018879e5421f93fc631e8edd15da')
cms-1  | 2024-05-22 23:47:11,200 INFO 327 [celery_utils.logged_task] [user 6] [ip 172.18.0.1] logged_task.py:25 - Task openedx.core.djangoapps.content.search.tasks.delete_xblock_index_doc[5f07654a-aeda-4b2d-b8f2-efd41ae30445] submitted with arguments ('block-v1:SampleTaxonomyOrg2+STC1+2023_1+type@vertical+block@f69a018879e5421f93fc631e8edd15da',), {}
cms-1  | 2024-05-22 23:47:11,214 ERROR 327 [edx_event_bus_redis.internal.producer] [user 6] [ip 172.18.0.1] producer.py:51 - Error delivering message to Redis event bus. error=Error -2 connecting to edx.devstack.redis:6379. Name or service not known. full_topic='dev-course-authoring-xblock-lifecycle' event_key=None signal=<OpenEdxPublicSignal: org.openedx.content_authoring.xblock.deleted.v1> initial_topic='course-authoring-xblock-lifecycle' event_key_field='xblock_info.usage_key' event_data={'xblock_info': XBlockData(usage_key=BlockUsageLocator(CourseLocator('SampleTaxonomyOrg2', 'STC1', '2023_1', None, None), 'vertical', 'f69a018879e5421f93fc631e8edd15da'), block_type='vertical', version=None)} event_metadata=EventsMetadata(event_type='org.openedx.content_authoring.xblock.deleted.v1', id=UUID('9b992d54-1895-11ef-9c09-0242ac120009'), minorversion=0, source='openedx/cms/web', sourcehost='eab1bef4bb21', time=datetime.datetime(2024, 5, 22, 23, 47, 11, 147475, tzinfo=datetime.timezone.utc), sourcelib=(9, 9, 2))

Copy link
Member

Choose a reason for hiding this comment

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

@pomegranited I encountered the same errors in my logs as well, however the deleted blocks do not show up in the search.

store = self._verify_modulestore_support(course_key, 'delete_item')
item = store.delete_item(location, user_id=user_id, **kwargs)
# .. event_implemented_name: XBLOCK_DELETED
XBLOCK_DELETED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=location,
block_type=location.block_type,

def send_deleted_event():
# .. event_implemented_name: XBLOCK_DELETED
XBLOCK_DELETED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=location,
block_type=location.block_type,
)
)
)

store.on_commit_changes_to(course_key, send_deleted_event)
return item

def revert_to_published(self, location, user_id):
Expand Down
Loading