-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Dashboard: don't prefetch latest build #12400
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?
Conversation
readthedocs/projects/models.py
Outdated
@@ -1100,23 +1098,17 @@ def full_find(self, filename, version): | |||
matches.append(os.path.join(root, match)) | |||
return matches | |||
|
|||
@lru_cache(maxsize=1) |
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.
this should also reduce the number of queries on .com, nothing like an old fashioned caching.
Tests for the number of queries should pass after readthedocs/ext-theme#639 is merged |
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 left some comments about a small change in behavior, but looks good otherwise.
@@ -399,7 +399,7 @@ def test_dashboard_number_of_queries(self): | |||
state=BUILD_STATE_FINISHED, | |||
) | |||
|
|||
with self.assertNumQueries(12): | |||
with self.assertNumQueries(27): |
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.
Maybe we want a note here explaining that more small fast queries here are better than a few slow ones.
@@ -30,7 +30,7 @@ def get_context_data(self, **kwargs): | |||
|
|||
# Show for the first few builds, return last build state | |||
if project.builds.count() <= 5: | |||
onboard["build"] = project.get_latest_build(finished=False) | |||
onboard["build"] = project.latest_internal_build |
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.
It seems there is some change in behavior here. We were returning unfinished builds before, and now we are returning anyone. Is that expected?
get_latest_build was doing something different than the prefetched value, looks like we never actually used this function with
finished=True
, so I went ahead and renamed it and made it a property so we can cache it.