-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Mjames/sol 174 #7313
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
Mjames/sol 174 #7313
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| """ module init will register signal handlers """ | ||
| import contentstore.signals |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| """ Code to allow module store to interface with courseware index """ | ||
| from __future__ import absolute_import | ||
|
|
||
| from datetime import timedelta | ||
| import logging | ||
|
|
||
| from django.conf import settings | ||
| from django.utils.translation import ugettext as _ | ||
| from eventtracking import tracker | ||
| from xmodule.modulestore import ModuleStoreEnum | ||
| from search.search_engine_base import SearchEngine | ||
|
|
||
|
|
||
| # Use default index and document names for now | ||
| INDEX_NAME = "courseware_index" | ||
| DOCUMENT_TYPE = "courseware_content" | ||
|
|
||
| # REINDEX_AGE is the default amount of time that we look back for changes | ||
| # that might have happened. If we are provided with a time at which the | ||
| # indexing is triggered, then we know it is safe to only index items | ||
| # recently changed at that time. This is the time period that represents | ||
| # how far back from the trigger point to look back in order to index | ||
| REINDEX_AGE = timedelta(0, 60) # 60 seconds | ||
|
|
||
| log = logging.getLogger('edx.modulestore') | ||
|
|
||
|
|
||
| def indexing_is_enabled(): | ||
| """ | ||
| Checks to see if the indexing feature is enabled | ||
| """ | ||
| return settings.FEATURES.get('ENABLE_COURSEWARE_INDEX', False) | ||
|
|
||
|
|
||
| class SearchIndexingError(Exception): | ||
| """ Indicates some error(s) occured during indexing """ | ||
|
|
||
| def __init__(self, message, error_list): | ||
| super(SearchIndexingError, self).__init__(message) | ||
| self.error_list = error_list | ||
|
|
||
|
|
||
| class CoursewareSearchIndexer(object): | ||
| """ | ||
| Class to perform indexing for courseware search from different modulestores | ||
| """ | ||
|
|
||
| @classmethod | ||
| def index_course(cls, modulestore, course_key, triggered_at=None, reindex_age=REINDEX_AGE): | ||
| """ | ||
| Process course for indexing | ||
|
|
||
| Arguments: | ||
| course_key (CourseKey) - course identifier | ||
|
|
||
| triggered_at (datetime) - provides time at which indexing was triggered; | ||
| useful for index updates - only things changed recently from that date | ||
| (within REINDEX_AGE above ^^) will have their index updated, others skip | ||
| updating their index but are still walked through in order to identify | ||
| which items may need to be removed from the index | ||
| If None, then a full reindex takes place | ||
|
|
||
| Returns: | ||
| Number of items that have been added to the index | ||
| """ | ||
| error_list = [] | ||
| searcher = SearchEngine.get_search_engine(INDEX_NAME) | ||
| if not searcher: | ||
| return | ||
|
|
||
| location_info = { | ||
| "course": unicode(course_key), | ||
| } | ||
|
|
||
| # Wrap counter in dictionary - otherwise we seem to lose scope inside the embedded function `index_item` | ||
| indexed_count = { | ||
| "count": 0 | ||
| } | ||
|
|
||
| # indexed_items is a list of all the items that we wish to remain in the | ||
| # index, whether or not we are planning to actually update their index. | ||
| # This is used in order to build a query to remove those items not in this | ||
| # list - those are ready to be destroyed | ||
| indexed_items = set() | ||
|
|
||
| def index_item(item, skip_index=False): | ||
| """ | ||
| Add this item to the search index and indexed_items list | ||
|
|
||
| Arguments: | ||
| item - item to add to index, its children will be processed recursively | ||
|
|
||
| skip_index - simply walk the children in the tree, the content change is | ||
| older than the REINDEX_AGE window and would have been already indexed. | ||
| This should really only be passed from the recursive child calls when | ||
| this method has determined that it is safe to do so | ||
| """ | ||
| is_indexable = hasattr(item, "index_dictionary") | ||
| item_index_dictionary = item.index_dictionary() if is_indexable else None | ||
| # if it's not indexable and it does not have children, then ignore | ||
| if not item_index_dictionary and not item.has_children: | ||
| return | ||
|
|
||
| item_id = unicode(item.scope_ids.usage_id) | ||
| indexed_items.add(item_id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martynjames I see what you're doing here, but it took me about 15 minutes before I realized why it is needed and why it still recursively calls
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added at initialization of |
||
| if item.has_children: | ||
| # determine if it's okay to skip adding the children herein based upon how recently any may have changed | ||
| skip_child_index = skip_index or \ | ||
| (triggered_at is not None and (triggered_at - item.subtree_edited_on) > reindex_age) | ||
| for child_item in item.get_children(): | ||
| index_item(child_item, skip_index=skip_child_index) | ||
|
|
||
| if skip_index or not item_index_dictionary: | ||
| return | ||
|
|
||
| item_index = {} | ||
| # if it has something to add to the index, then add it | ||
| try: | ||
| item_index.update(location_info) | ||
| item_index.update(item_index_dictionary) | ||
| item_index['id'] = item_id | ||
| if item.start: | ||
| item_index['start_date'] = item.start | ||
|
|
||
| searcher.index(DOCUMENT_TYPE, item_index) | ||
| indexed_count["count"] += 1 | ||
| except Exception as err: # pylint: disable=broad-except | ||
| # broad exception so that index operation does not fail on one item of many | ||
| log.warning('Could not index item: %s - %r', item.location, err) | ||
| error_list.append(_('Could not index item: {}').format(item.location)) | ||
|
|
||
| def remove_deleted_items(): | ||
| """ | ||
| remove any item that is present in the search index that is not present in updated list of indexed items | ||
| as we find items we can shorten the set of items to keep | ||
| """ | ||
| response = searcher.search( | ||
| doc_type=DOCUMENT_TYPE, | ||
| field_dictionary={"course": unicode(course_key)}, | ||
| exclude_ids=indexed_items | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martynjames I'm a bit concerned of how it would behave for actual course with many XBlocks. If I understand right, under the hood it would build an enormous Also, the deletion approach is not the most efficient one as well - it deletes one document at a time, while there are more efficient approach built in: delete by query (we're using 0.90 in devstack and prodstack, right?). It would be nice to have an ability to use bulk operations with search (where it makes sense). Not insisting on it though, and not a blocker for this PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I believe this should be a performance improvement follow-up task.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have created https://openedx.atlassian.net/browse/SOL-641 to prioritize and track this activity specifically
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some courses can have thousands of usage keys. I'm not as concerned with speed if this is happening asynchronously, but will this interface error if there are that many
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a test to investigate when it would start to error (however it is disabled for running on Jenkins because it takes time to build a course that is very large). I ran last night with ever-increasing sizes of courses for 10-12 hours with no failures. To get an idea of how far I got I ran the test with approx. 4096 items in the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ormsbee - went up to 10,000 items in exclude_id with no errors, so I think it is (at least practically) safe. |
||
| ) | ||
| result_ids = [result["data"]["id"] for result in response["results"]] | ||
| for result_id in result_ids: | ||
| searcher.remove(DOCUMENT_TYPE, result_id) | ||
|
|
||
| try: | ||
| with modulestore.branch_setting(ModuleStoreEnum.RevisionOption.published_only): | ||
| course = modulestore.get_course(course_key, depth=None) | ||
| for item in course.get_children(): | ||
| index_item(item) | ||
| remove_deleted_items() | ||
| except Exception as err: # pylint: disable=broad-except | ||
| # broad exception so that index operation does not prevent the rest of the application from working | ||
| log.exception( | ||
| "Indexing error encountered, courseware index may be out of date %s - %r", | ||
| course_key, | ||
| err | ||
| ) | ||
| error_list.append(_('General indexing error occurred')) | ||
|
|
||
| if error_list: | ||
| raise SearchIndexingError(_('Error(s) present during indexing'), error_list) | ||
|
|
||
| return indexed_count["count"] | ||
|
|
||
| @classmethod | ||
| def do_course_reindex(cls, modulestore, course_key): | ||
| """ | ||
| (Re)index all content within the given course, tracking the fact that a full reindex has taking place | ||
| """ | ||
| indexed_count = cls.index_course(modulestore, course_key) | ||
| if indexed_count: | ||
| cls._track_index_request('edx.course.index.reindexed', indexed_count) | ||
| return indexed_count | ||
|
|
||
| @classmethod | ||
| def _track_index_request(cls, event_name, indexed_count): | ||
| """Track content index requests. | ||
|
|
||
| Arguments: | ||
| event_name (str): Name of the event to be logged. | ||
| Returns: | ||
| None | ||
|
|
||
| """ | ||
| data = { | ||
| "indexed_count": indexed_count, | ||
| 'category': 'courseware_index', | ||
| } | ||
|
|
||
| tracker.emit( | ||
| event_name, | ||
| data | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| """ receiver of course_published events in order to trigger indexing task """ | ||
| from datetime import datetime | ||
| from pytz import UTC | ||
|
|
||
| from django.dispatch import receiver | ||
|
|
||
| from xmodule.modulestore.django import SignalHandler | ||
| from contentstore.courseware_index import indexing_is_enabled | ||
|
|
||
|
|
||
| @receiver(SignalHandler.course_published) | ||
| def listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument | ||
| """ | ||
| Receives signal and kicks off celery task to update search index | ||
| """ | ||
| # import here, because signal is registered at startup, but items in tasks are not yet able to be loaded | ||
| from .tasks import update_search_index | ||
| if indexing_is_enabled(): | ||
| update_search_index.delay(unicode(course_key), datetime.now(UTC).isoformat()) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,35 @@ | ||
| """ | ||
| This file contains celery tasks for contentstore views | ||
| """ | ||
|
|
||
| from celery.task import task | ||
| from django.contrib.auth.models import User | ||
| import json | ||
| import logging | ||
| from xmodule.modulestore.django import modulestore | ||
| from xmodule.course_module import CourseFields | ||
| from celery.task import task | ||
| from celery.utils.log import get_task_logger | ||
| from datetime import datetime | ||
| from pytz import UTC | ||
|
|
||
| from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError | ||
| from course_action_state.models import CourseRerunState | ||
| from django.contrib.auth.models import User | ||
|
|
||
| from contentstore.courseware_index import CoursewareSearchIndexer, SearchIndexingError | ||
| from contentstore.utils import initialize_permissions | ||
| from course_action_state.models import CourseRerunState | ||
| from opaque_keys.edx.keys import CourseKey | ||
| from xmodule.course_module import CourseFields | ||
| from xmodule.modulestore.django import modulestore | ||
| from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError | ||
|
|
||
| from edxval.api import copy_course_videos | ||
| LOGGER = get_task_logger(__name__) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @martynjames nit: correct me if I'm wrong, but logger here is not a constant, and all caps are reserved for constants. It's not modified within the module, so it essentially is a constant, but it's more like a module memeber as it's instantiated as a result of function call. Also (primary argument), I haven't seen module-level loggers in all caps anywhere in the code body.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latest pylint requires that globals have UPPERCASE names, not constants - it's either this or add a pylint ignore flag. |
||
| FULL_COURSE_REINDEX_THRESHOLD = 1 | ||
|
|
||
|
|
||
| @task() | ||
| def rerun_course(source_course_key_string, destination_course_key_string, user_id, fields=None): | ||
| """ | ||
| Reruns a course in a new celery task. | ||
| """ | ||
| # import here, at top level this import prevents the celery workers from starting up correctly | ||
| from edxval.api import copy_course_videos | ||
|
|
||
| try: | ||
| # deserialize the payload | ||
| source_course_key = CourseKey.from_string(source_course_key_string) | ||
|
|
@@ -72,3 +80,21 @@ def deserialize_fields(json_fields): | |
| for field_name, value in fields.iteritems(): | ||
| fields[field_name] = getattr(CourseFields, field_name).from_json(value) | ||
| return fields | ||
|
|
||
|
|
||
| @task() | ||
| def update_search_index(course_id, triggered_time_isoformat): | ||
| """ Updates course search index. """ | ||
| try: | ||
| course_key = CourseKey.from_string(course_id) | ||
| triggered_time = datetime.strptime( | ||
| # remove the +00:00 from the end of the formats generated within the system | ||
| triggered_time_isoformat.split('+')[0], | ||
| "%Y-%m-%dT%H:%M:%S.%f" | ||
| ).replace(tzinfo=UTC) | ||
| CoursewareSearchIndexer.index_course(modulestore(), course_key, triggered_at=triggered_time) | ||
|
|
||
| except SearchIndexingError as exc: | ||
| LOGGER.error('Search indexing error for complete course %s - %s', course_id, unicode(exc)) | ||
| else: | ||
| LOGGER.debug('Search indexing successful for complete course %s', course_id) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martynjames one minor suggestion (and sorry I didn't brought this in earlier) - could you please convert it to normal class with instance or class methods? If you noticed I've done exactly so in https://github.com/edx/edx-platform/pull/7448 - libraries are indexed a little bit different, so (1) correct indexer need to be picked up based on what we are indexing and (2) most of the code in this class can be reused, but with some overrides. So if you could convert this to classmethods it would be a bit simpler for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martynjames thank you so much!