Skip to content
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

Cache language autocomplete inputs/endpoints #9112

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Apr 18, 2024

Closes #9111

Technical

Removes timestamp because it is not used and would mess up caching.

Testing

Compare the speed of language on prod and testing for a work like

As far as I can tell, it is drastically faster.

Screenshot

Stakeholders

@RayBB RayBB added Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing labels Apr 18, 2024
@mekarpeles mekarpeles self-assigned this Apr 22, 2024
@mekarpeles mekarpeles merged commit c1d6788 into master Apr 22, 2024
4 checks passed
@cdrini cdrini deleted the feat/improve-language-autocomplete branch April 30, 2024 18:30
@cdrini
Copy link
Collaborator

cdrini commented Apr 30, 2024

Note: This will cache for all autocompletion endpoints

@cdrini cdrini removed the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label Apr 30, 2024
@cdrini cdrini changed the title improve language autocomplete speed Cache all autocomplete inputs Apr 30, 2024
@cdrini
Copy link
Collaborator

cdrini commented Apr 30, 2024

This might be non-ideal in certain cases ; e.g. when new works are created.

@cdrini cdrini changed the title Cache all autocomplete inputs Cache all autocomplete inputs/endpoints Apr 30, 2024
@@ -96,6 +96,7 @@ class languages_autocomplete(delegate.page):
def GET(self):
i = web.input(q="", limit=5)
i.limit = safeint(i.limit, 5)
web.header("Cache-Control", "max-age=%d" % (24 * 3600))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is little value in caching this ; it's very unlikely a user will type the same thing twice! In general, it's not super likely a user will type the same thing many times. I imagine a veeeeeery tiny percentage of user searches will be cache hits; most will be misses. I think we might want to roll this back, and cache it only in the case we care about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdrini this comes from a librarian commenting of the lag that happens when searching languages. I don't know what exactly they're editing that they see it often enough to comment.

How about after the deploy goes how to we check how many requests this endpoint is getting on average? Also we can follow up with that librarian to see what their use case it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw this caching works across pages so if someone edits one book to add a language the next time they do that on any book it'll use the cache.

@RayBB
Copy link
Collaborator Author

RayBB commented Apr 30, 2024

@cdrini actually, now I realize the title is wrong and there's a bit of a misunderstanding. This is only for the language auto complete endpoint. Doesn't effect works or authors search.

@RayBB RayBB changed the title Cache all autocomplete inputs/endpoints Cache language autocomplete inputs/endpoints Apr 30, 2024
@cdrini
Copy link
Collaborator

cdrini commented May 8, 2024

Ah my bad! The JS change affects all endpoints, but it looks like we don't cache this endpoint in any way, so the timestamp cache-bust serves no purpose and can be removed. And yep it looks like the python changes is only for the languages endpoint. My bad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edition Editing: language autocomplete is slow
3 participants