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

Harden download robustness #5545

Merged
merged 21 commits into from Nov 27, 2017

Conversation

Projects
None yet
1 participant
@benjaoming
Member

benjaoming commented Nov 20, 2017

Summary

  • Fixes blank video downloads when server is down (part of #5538)
  • Raises exceptions when non-200 download responses
  • Resumes video / content pack download in case of connection disruptions during download (not in case the download is stopped/started)
  • Adds tests for various failures
  • Adds tests for downloading + deleting content packs
  • Deletes content pack database when deleting a content pack.
  • Fixed missing update from database when a video download gets canceled while being processed
  • Improves logging on many fronts
  • Feedback when downloading content packs #5356

TODO

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

  • Has documentation been written/updated?
  • Have you written release notes for the upcoming release?
  • Allow to fail gracefully if thumbnails are absent

Reviewer guidance

...

Issues addressed

#5538
#5356

Harden download robustness: Raise exceptions when non-200 download re…
…sponses, add tests for various failures, improve logging

@benjaoming benjaoming added this to the 0.17.4 milestone Nov 20, 2017

@benjaoming benjaoming referenced this pull request Nov 20, 2017

Closed

Video download robustness improvements #5538

5 of 6 tasks complete
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 20, 2017

Codecov Report

Merging #5545 into 0.17.x will increase coverage by 1.89%.
The diff coverage is 73.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.17.x    #5545      +/-   ##
==========================================
+ Coverage   64.67%   66.56%   +1.89%     
==========================================
  Files         117      117              
  Lines        6612     6639      +27     
==========================================
+ Hits         4276     4419     +143     
+ Misses       2336     2220     -116
Impacted Files Coverage Δ
kalite/updates/settings.py 100% <ø> (ø) ⬆️
kalite/topic_tools/content_models.py 77.8% <0%> (-0.29%) ⬇️
kalite/updates/models.py 84.82% <100%> (+0.13%) ⬆️
kalite/updates/management/utils.py 78.44% <100%> (+5.88%) ⬆️
...alite/updates/management/commands/videodownload.py 74.56% <62.5%> (+15.73%) ⬆️
kalite/updates/videos.py 88.73% <73.33%> (+2.79%) ⬆️
...ributed/management/commands/retrievecontentpack.py 89.16% <76.92%> (+40.98%) ⬆️
kalite/i18n/base.py 76.17% <81.25%> (+20.31%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86e1ea6...e941add. Read the comment docs.

codecov bot commented Nov 20, 2017

Codecov Report

Merging #5545 into 0.17.x will increase coverage by 1.89%.
The diff coverage is 73.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.17.x    #5545      +/-   ##
==========================================
+ Coverage   64.67%   66.56%   +1.89%     
==========================================
  Files         117      117              
  Lines        6612     6639      +27     
==========================================
+ Hits         4276     4419     +143     
+ Misses       2336     2220     -116
Impacted Files Coverage Δ
kalite/updates/settings.py 100% <ø> (ø) ⬆️
kalite/topic_tools/content_models.py 77.8% <0%> (-0.29%) ⬇️
kalite/updates/models.py 84.82% <100%> (+0.13%) ⬆️
kalite/updates/management/utils.py 78.44% <100%> (+5.88%) ⬆️
...alite/updates/management/commands/videodownload.py 74.56% <62.5%> (+15.73%) ⬆️
kalite/updates/videos.py 88.73% <73.33%> (+2.79%) ⬆️
...ributed/management/commands/retrievecontentpack.py 89.16% <76.92%> (+40.98%) ⬆️
kalite/i18n/base.py 76.17% <81.25%> (+20.31%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86e1ea6...e941add. Read the comment docs.

benjaoming added some commits Nov 23, 2017

Add download feedback for retrievecontentpack + Only keep retrying fo…
…r about 1 minute before giving up in download_file
@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 23, 2017

Member

Added download progress for contentpack retrieval.

image

Member

benjaoming commented Nov 23, 2017

Added download progress for contentpack retrieval.

image

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 23, 2017

Member

Download progress callback also added in UI

image

Member

benjaoming commented Nov 23, 2017

Download progress callback also added in UI

image

@benjaoming benjaoming referenced this pull request Nov 24, 2017

Closed

PEX test file build script + buildkite integration #5544

0 of 3 tasks complete
@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 24, 2017

Member

Repositioned the progress bars for content pack downloads so it doesn't look like something is broken.

image

Member

benjaoming commented Nov 24, 2017

Repositioned the progress bars for content pack downloads so it doesn't look like something is broken.

image

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 24, 2017

Member

A small related TODO of this PR would be to add automated testing for retrieving and deleting a content pack.

Member

benjaoming commented Nov 24, 2017

A small related TODO of this PR would be to add automated testing for retrieving and deleting a content pack.

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Nov 27, 2017

Member

Merging this for further tests in a pre-release!

Member

benjaoming commented Nov 27, 2017

Merging this for further tests in a pre-release!

@benjaoming benjaoming merged commit 25c954e into learningequality:0.17.x Nov 27, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 73.14% of diff hit (target 64.67%)
Details
codecov/project 66.56% (+1.89%) compared to 86e1ea6
Details

@benjaoming benjaoming deleted the benjaoming:bug/video_url branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment