refactor: convert global modulestore to request-cached #37895
Conversation
Having a global modulestore reference is a holdover from the early days, when the XML modulestore would be extremely expensive to initialize. That is no longer the case, and we should eliminate the global here to reduce the chances of multi-threading bugs that might mutate the global. Unfortunately, a lot of code currently assumes that calls to the modulestore() function are essentially free, instead of the 1-2 ms it is without caching. There are cases where this may be called thousands of times deep in loops somewhere while doing course content traversal, so it's risky to eliminate the caching behavior altogether. The thought here is that we'll tie to the RequestCache, which should at least not be shared across users/threads. Since new instances of the modulestore will not have cached entries for a particular course, it means that some query count tests have to be adjusted upward.
ormsbee
requested review from
a team,
farhan,
irtazaakram and
salman2013
as code owners
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
Makes sense, I wasn't sure about the motivations for having a separate namespace for the request cache but that's not really a blocker, just looking to learn more.
| log = logging.getLogger(__name__) | ||
| ASSET_IGNORE_REGEX = getattr(settings, "ASSET_IGNORE_REGEX", r"(^\._.*$)|(^\.DS_Store$)|(^.*~$)") | ||
|
|
||
| MODULESTORE_REQUEST_CACHE_NAMESPACE = "modulestore" |
There was a problem hiding this comment.
This is just for my own curiosity, why namespace this separately? Is it likely that the modulestore cache needs to be cleared independently of the rest of the request cache? Either at a different cadence or after certain events?
There was a problem hiding this comment.
Mostly because it can be cleared separately in some tests, and it's cheap to separate it.
|
@feanil: These tests seem to be hanging. I thought it might be a holdover from the CI issues we had the other day, but it looks like it's still spinning/hanging. I'll see if it'll also hang my local machine running the suite... |
|
(sigh, I mis-clicked "close") |
|
For anyone following this, I closed it because the tests go into an infinite loop that will chew up all the CPU on a machine. I suspect this has to do with grabbing MongoDB connections, but I have yet to verify. It takes a long test suite to work up to it, and I haven't yet crafted a single test to replicate it (though I have an idea for that). Since debugging this will likely take some time, I'm parking it here for now. |
2 participants
A re-push of #37859 because I think something in the CI for that PR is broken.