-
-
Notifications
You must be signed in to change notification settings - Fork 182
fix(search): Refactor cl.lib.search_utils.fetch_and_paginate_results … #5931
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
base: main
Are you sure you want to change the base?
fix(search): Refactor cl.lib.search_utils.fetch_and_paginate_results … #5931
Conversation
…to cache only ES Response objects and create Paginator.Page objects from the results. Requires enriching the results after fetching from the cache, so there is still additional possible performance increase by refactoring enrichment functions to process the ES Response rather than the Page objects. Fixes freelawproject#5562.
for more information, see https://pre-commit.ci
Thank you @amattalols! I'm assigning to @albertisfu for review during this sprint (which ends Friday). |
Alberto, I put this on your todo, but perhaps we should push until Jamaica sprint. If so, no worries, just go ahead and push it if you want. :) |
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.
Thanks, @amattalols, for your work on this issue. The PR looks great! just a minor comment to address.
cl/lib/search_utils.py
Outdated
|
||
elif type(cache_data) is Response: | ||
# Create Django paginator for insights as ES metadata is not stored | ||
paginator = Paginator(cache_data, page) |
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.
I think we need to use rows_per_page
here; otherwise, only one result will be displayed on the home page for the latest opinions.
paginator = Paginator(cache_data, page) | |
paginator = Paginator(cache_data, rows_per_page) |
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.
Yep, you're right. Fixed in latest commit.
…to cache only ES Response objects and create Paginator.Page objects from the results. Requires enriching the results after fetching from the cache, so there is still additional possible performance increase by refactoring enrichment functions to process the ES Response rather than the Page objects.
Fixes #5562.