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

Use media availability instead of querying the filesystem #6428

Merged
merged 1 commit into from Dec 3, 2019

Conversation

@davidfischer
Copy link
Contributor

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 readthedocs/core Dec 3, 2019
@humitos
humitos approved these changes Dec 3, 2019
Copy link
Member

humitos left a comment

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

This comment has been minimized.

Copy link
@humitos
Copy link
Member

ericholscher left a comment

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
3 checks passed
3 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
@ericholscher ericholscher deleted the davidfischer/use-cached-media-availability branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.