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

Prefetch build and project on version list #11616

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Sep 23, 2024

This optimizes the version listing view and removes redundant queries.

- Requires readthedocs/ext-theme#493
- Fixes readthedocs/ext-theme#463
Comment on lines +305 to +306
if hasattr(self, self.LATEST_BUILD_CACHE):
if latest_build := getattr(self, self.LATEST_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.

I don't think we need the first if here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started to write it like that but we do need to test for hasattr first, as this attribute only exists after prefetch and it's what shortcuts this method to avoid the return self.builds.... below.

If this was just getattr, we'd always skip the return self.builds... below.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍🏼

@@ -141,6 +141,30 @@ def for_reindex(self):
.distinct()
)

def prefetch_subquery(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we call it prefetch_latest_build() instead to keep consistency to what we are doing with Project at https://github.com/readthedocs/readthedocs.org/pull/11613/files#diff-05611882195df182851df952b312f031a4314bce04a104e10a09655e570515f3R124?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That method shouldn't be called prefetch_latest_build, it's doing more than that. Similar here, there are other fields to prefetch.

@humitos
Copy link
Member

humitos commented Sep 25, 2024

I'm going to merge it so it goes out in today's deploy, but there are some comments that need to be addressed.

@humitos humitos merged commit e5f8092 into main Sep 25, 2024
6 checks passed
@humitos humitos deleted the agj/version-list-prefetch branch September 25, 2024 10:25
agjohnson added a commit that referenced this pull request Sep 25, 2024
agjohnson added a commit that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants