Skip to content

Add support for successful build prefetch #11613

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 25, 2024
Merged

Add support for successful build prefetch #11613

merged 1 commit into from
Sep 25, 2024

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 23, 2024 •
edited
Loading

We use both latest build and latest successful build in the project
listing template.

@agjohnson
Add support for successful build prefetch
95d3106 
We use both latest build and latest successful build in the project
listing template.
@agjohnson agjohnson requested a review from a team as a code owner September 23, 2024 20:00
@agjohnson agjohnson requested a review from humitos September 23, 2024 20:00
@agjohnson
Copy link
Contributor Author

agjohnson commented Sep 23, 2024 •
edited
Loading

This dropped the query count on the project listing page from 85 to 27 for me. Project.has_good_build was incurring an extra 3 queries per table row because it was avoiding the prefetched build query.

@agjohnson agjohnson mentioned this pull request Sep 23, 2024
Closed
ericholscher
ericholscher approved these changes Sep 24, 2024
View reviewed changes
readthedocs/projects/models.py
Comment on lines +926 to +927
if hasattr(self, self.LATEST_SUCCESSFUL_BUILD_CACHE):
if build_successful := getattr(self, self.LATEST_SUCCESSFUL_BUILD_CACHE):
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback about not needing the first if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted on #11616 but hasattr does indeed seem needed as hasattr() == False is what signals that prefetching is not in use and the normal method query can be run.

@humitos
Copy link
Member

humitos commented Sep 25, 2024

Merging to go out today, but there are some comments that need to be addressed.

@humitos humitos merged commit a6e1fde into main Sep 25, 2024
8 checks passed
@humitos humitos deleted the agj/project-list-prefetch branch September 25, 2024 10:26
@sentry-io Sentry.io

This comment was marked as off-topic.

Sign in to view
agjohnson added a commit that referenced this pull request Sep 25, 2024
@agjohnson
Revert "Add support for successful build prefetch (#11613)"
aae5718 
This reverts commit a6e1fde.
@agjohnson agjohnson mentioned this pull request Sep 26, 2024
Merged
agjohnson added a commit that referenced this pull request Sep 26, 2024
@agjohnson
Revert "Add support for successful build prefetch (#11613)"
bbec81c 
This reverts commit a6e1fde.
agjohnson added a commit that referenced this pull request Sep 30, 2024
@agjohnson
Try reverting prefetch changes for project/version listing views (#11621
6569f5f 
)

* Revert "Prefetch build and project on version list (#11616)"

This reverts commit e5f8092.

* Revert "Add support for successful build prefetch (#11613)"

This reverts commit a6e1fde.

* Don't use project prefetching for now

* Still rename model method, without prefetch

* Add comment back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericholscher ericholscher ericholscher approved these changes

@humitos humitos Awaiting requested review from humitos humitos is a code owner automatically assigned from readthedocs/backend

Assignees

@agjohnson agjohnson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add prefetch/cache/async load for builds on project dashboard
3 participants
@agjohnson @humitos @ericholscher