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

Only download videos not already downloaded #5016

Conversation

MCGallaspy
Copy link
Contributor

Summary

Filters the requested ids to download. I think this could potentially increase the runtime of this endpoint significantly, since each youtube_id hits the database -- but maybe it could be improved by a big constant factor if all the content items are fetched at once? Alternatively, if the requests are batched on the front-end. Thoughts?

TODO

If not all TODOs are marked, this PR is considered WIP (work in progress)

  • Have tests been written for the new code? If you're fixing a bug, write a regression test (or have a really good reason for not writing one... and I mean really good!)

Issues addressed

Fixes #4990.

@MCGallaspy MCGallaspy added the bug label Mar 17, 2016
@MCGallaspy MCGallaspy added this to the 0.16.0 milestone Mar 17, 2016
@@ -129,10 +130,12 @@ def start_video_download(request):
lang = json.loads(request.body or "{}").get("lang", "en")

youtube_ids = get_download_youtube_ids(paths, language=lang)

ids_to_queue = {id_: title for (id_, title) in youtube_ids.iteritems()

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Allows the UpdateProgressLog for video downloads/deletes to be
accurately reported, as well as avoiding unnecessary downloads.
@MCGallaspy MCGallaspy force-pushed the 4990-video-count-mismatch branch from 6489044 to 880e885 Compare March 18, 2016 21:32
@MCGallaspy
Copy link
Contributor Author

@rtibbles in the interest of time, I'm compromising my test policy.

This was referenced Mar 23, 2016
@rtibbles
Copy link
Member

I still don't think checking the file system is the right fix. We should, perhaps, check that at download, but the point of flagging these things in the database is so that we don't have to constantly check the file system.

I have implemented a fix for this in my fix for the download availability issue for different languages, so will close this when that is opened.

@rtibbles rtibbles mentioned this pull request Mar 25, 2016
4 tasks
@rtibbles
Copy link
Member

Closing in preference to #5045.

@rtibbles rtibbles closed this Mar 25, 2016
@rtibbles rtibbles self-assigned this Mar 25, 2016
@rtibbles rtibbles removed the has PR label Mar 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants