Skip to content

Use media availability instead of querying the filesystem #6428

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

Merged
merged 1 commit into from
Dec 3, 2019

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Dec 3, 2019

This change allows us to query the database instead of the file system or remote storage to get whether a version has a download of a specific type.

Currently, we rely on the presence of a file existing at a certain filesystem or storage path to determine whether media/downloads are available. As we move toward storage being an abstraction on top of the filesystem where it is possibly remote (eg. Cloud storage) we don't want to have to query remote storage to know whether a version has a PDF.

Instead, we started caching whether a version has builds of a specific type (eg. a PDF, ePub) and storing it on the Version model. After we started storing that data, a script was run to store the results for all previously built versions.

This builds on work in #6278

Future work

There's some additional work I didn't add into this PR to keep it minimal. However, we could add this cleanup if desired:

  • Remove the Project.get_downloads method which gets project downloads for the default version. As far as I can tell, this is not used.
  • Remove the Project.has_pdf, Project.has_epub, and Project.has_htmlzip methods. These do not appear to be used anymore. Alternatively, these could be changed to get the version object and query it instead of hitting the filesystem/storage. I think it really depends on whether we want to maintain this API.

This change allows us to query the database instead of the file system
or remote storage to get whether a verison has a download
of a specific type.
@davidfischer davidfischer requested a review from a team December 3, 2019 04:36
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.

LGTM!

I'm 👍 on removing all the methods you mentioned in the description if they are not used anymore. I don't see any value on keeping them around. In the end they only add confusion when reading the code.

@@ -12,7 +12,6 @@
from readthedocs.builds.models import Version
from readthedocs.core.middleware import FooterNoSessionMiddleware
from readthedocs.projects.models import Project
from readthedocs.rtd_tests.mocks.paths import fake_paths_by_regex
Copy link
Member

Choose a reason for hiding this comment

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

😮

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks great to me, excited to get it out. 👍

Agreed about removing the methods, lets keep things simpler and clean up the model methods if we aren't using them.

@ericholscher ericholscher merged commit 1626626 into master Dec 3, 2019
@ericholscher ericholscher deleted the davidfischer/use-cached-media-availability branch December 3, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants