ENT-2168 | setting cache for get_catalog_results from discovery #545
Conversation
HammadAhmadWaqas
changed the title
HammadAhmadWaqas
force-pushed
the
hammad/ENT-2168
branch
from
2256e6e to
f4c9ee4
Compare
enterprise/api_client/discovery.py
Outdated
| query_params, | ||
| traverse_pagination | ||
| ) | ||
| cache.set(cache_key, response, settings.DISCOVERY_GET_CATALOG_RESULT_CACHE_TIMEOUT) |
There was a problem hiding this comment.
instead of creating DISCOVERY_GET_CATALOG_RESULT_CACHE_TIMEOUT in this we should use ENTERPRISE_API_CACHE_TIMEOUT.
There was a problem hiding this comment.
I think ENTERPRISE_API_CACHE_TIMEOUT is used for API that belongs to Enterprise. That is why we are using it in EnterpriseApiClient.
But now we want some TimeOut period for Discovery to use in CourseCatalogApiClient.
There was a problem hiding this comment.
That time out ENTERPRISE_API_CACHE_TIMEOUT is used in cache results of discovery call. since that enterprise API is calling discovery.
enterprise/api_client/discovery.py
Outdated
| @@ -116,10 +118,20 @@ def traverse_pagination(response, endpoint, content_filter_query, query_params): | |||
|
|
|||
| return results | |||
|
|
|||
| def get_catalog_results(self, content_filter_query, query_params=None, traverse_pagination=False): | |||
| def _get_catalog_results_from_discovery(self, content_filter_query, query_params=None, traverse_pagination=False): | |||
There was a problem hiding this comment.
Since we are using _get_catalog_results_from_discovery in tests can we remove underscore prefix from its name?
HammadAhmadWaqas
force-pushed
the
hammad/ENT-2168
branch
2 times, most recently
from
3391db5 to
95f6f7c
Compare
HammadAhmadWaqas
force-pushed
the
hammad/ENT-2168
branch
from
95f6f7c to
2420d53
Compare
There was a problem hiding this comment.
@HammadAhmadWaqas Thank you for the quick turn around on this fix.
I believe we should also set the cache on the _load_data function. That's where a bunch of other catalog api calls are piped through, like get_course_run and get_course_id. Could you please add your cache mechanism on those as well?
|
@schenedx it appears |
There was a problem hiding this comment.
@HammadAhmadWaqas I didn't know that. It explains about the difference of logs I see between edx-enterprise calls on discovery, and the actual discovery logs.
Let's deploy this change and see if this indeed fixes the performance issue associated with enterprise course enrollment.
…a-to-4.2.4 chore: upgrade edx-enterprise-data to 4.2.4
Description: setting cache for get_catalog_results from discovery
JIRA: https://openedx.atlassian.net/browse/ENT-2168
Dependencies: dependencies on other outstanding PRs, issues, etc.
Merge deadline: List merge deadline (if any)
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
Merge checklist:
base.inif needed in production, but edx-platform doesn't install it.test-master.inif edx-platform pins it, with a matching version.make upgrade && make requirements(and make sure to fix any errors).DO NOT just add dependencies to
requirements/*.txtfiles.make staticfor webpack bundling if any static content was updated.Post merge:
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.