perf: request cache get_schedule_for_user #259
Conversation
|
Note to self: test failures are probably because we need to clear the request cache between tests. |
| @@ -526,16 +529,15 @@ def test_get_dates_for_course_query_counts(self, has_schedule, pass_user_object, | |||
| course_id=self.course.id, user=user, schedule=schedule | |||
| ) | |||
|
|
|||
| # Second time, the request cache eliminates all querying (sometimes)... | |||
| # If a schedule is not provided, we will get the schedule to avoid caching outdated dates | |||
There was a problem hiding this comment.
... I'm not sure if it's dangerous that I'm altering this particular behavior, i.e. if there's something that assumes schedules are never cached.
|
@ilee2u, @zacharis278, @MichaelRoytman: Would one of you have time to review this? It was reported as a performance issue on the forum. I imagine edx.org would also seeing the same issue. (The failing check at the moment is codecov rate limiting.) |
ormsbee
added
the
performance
Calls to render a Unit were calling this function many times, even when schedules are not enabled. This was leading to noticeable performance issues in production, particularly for Units with many components.
| if not Schedule: | ||
| return None |
There was a problem hiding this comment.
This is not covered by any tests at this moment, because it never happens in practice–either we're running inside the edx-platform process and the import succeeds, or we're in edx-when tests and it's been mocked with a DummySchedule object.
@ilee2u: I'm inclined to just ignore this omission (no test existed for the similar check it replaced), but I can add a test for this if you think it's warranted.
|
Hi folks. I'm back from PTO and looking to get this landed. I rebased just to change the update date for the version bump in the CHANGELOG.rst. The unit test error is there because of codecov rate limiting. In the previous run where that test did run, the one thing it flagged was this issue. @ilee2u: I do have access to override the checks in this case, but I'll defer to your judgement as to what's appropriate here. I'll try re-running the test late tonight to see if it goes through okay. Thank you. |
Calls to render a Unit were calling this function many times, even when
schedules are not enabled. This was leading to noticeable performance
issues in production, particularly for Units with many components.
I still need to test this properly, which is probably going to involve
hacky looking mocking because of the weird model relationship this has
with edx-platform (where it's actually importing the Schedule model
from edx-platform).