perf: wrap reindex_course loop in modulestore().bulk_operations - #39134
holaontiveros wants to merge 1 commit into
Conversation
Sorry, something went wrong.
The reindex_course management command indexes each course by calling CoursewareSearchIndexer.do_course_reindex(store, course_key) in a bare loop. Without a bulk_operations context the split modulestore does not cache the course structure across the block-tree walk the indexer performs, so the (potentially multi-megabyte) structure is re-read from MongoDB on every block access. On large courses this dominates the reindex time. The Studio reindex HTTP handler already wraps the same call in modulestore().bulk_operations(course_key) (cms/djangoapps/contentstore/views/course.py course_search_index_handler); this change applies the same wrapper to the management command so that `reindex_course --all` / `--active` benefit from the single-fetch/cached structure path too. Measured on a WGU (Ulmo) deployment: a large course dropped from ~90s to ~5s per course (~18x) when wrapped, matching the Studio "Reindex" button's time. Refs: openedx#36868 Adds a regression test asserting each course reindex runs inside store.bulk_operations(course_key).
|
Thanks for the pull request, @holaontiveros! This repository is currently maintained by @openedx/wg-maintenance-openedx-platform-oncall. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information? If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Sorry, something went wrong.
|
@bradenmacdonald @ormsbee this came from looking improvements on the indexing part and doing some tests, we needed to do the course index part and I noticed that doing it on from the studio frontend took seconds but if i tried to do it by the management command it was minutes |
Sorry, something went wrong.
Description
The reindex_course management command indexes each course with a bare
CoursewareSearchIndexer.do_course_reindex(store, course_key) call in a loop:
Without a bulk_operations context, the split modulestore does not cache the
course structure across the block-tree walk the indexer performs, so the
(potentially multi-megabyte) course structure is re-read from MongoDB on every
block access. On large courses this dominates the reindex time.
The Studio reindex HTTP handler already wraps the same call in
modulestore().bulk_operations(course_key)
(cms/djangoapps/contentstore/views/course.py, course_search_index_handler).
This PR applies the same wrapper to the management command so
reindex_course --all / --active get the single-fetch, cached-structure path
too.
Impact
Measured on an Ulmo deployment: a large course went from ~90s to ~5s per
course (~18x) once wrapped — matching the Studio "Reindex" button, which already
has the wrapper. Smaller courses see a proportional improvement.
Change
Plus a regression test asserting each course reindex runs inside
store.bulk_operations(course_key).
Refs #36868
Testing
store's bulk_operations and asserts it is entered for each course key.
the mocked do_course_reindex).
Note: I was unable to run the full CMS test suite in my environment; the change
is minimal and mirrors the existing, tested handler pattern. Reviewers/CI please
confirm.