Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
store: do not resume a download when we already have the whole thing #3640
Conversation
|
@zyga better? :-) |
codecov-io
commented
Aug 2, 2017
•
Edited 1 time
-
codecov-io
Aug 3, 2017
-
Aug 3, 2017
Codecov Report
@@ Coverage Diff @@
## master #3640 +/- ##
==========================================
- Coverage 75.2% 75.19% -0.02%
==========================================
Files 387 388 +1
Lines 33452 33491 +39
==========================================
+ Hits 25159 25183 +24
- Misses 6481 6494 +13
- Partials 1812 1814 +2
Continue to review full report at Codecov.
|
zyga
requested a review
from
pedronis
Aug 3, 2017
|
Much better, thank you! |
| @@ -1359,7 +1359,22 @@ func (s *Store) Download(ctx context.Context, name string, targetPath string, do | ||
| url = downloadInfo.DownloadURL | ||
| } | ||
| - err = download(ctx, name, downloadInfo.Sha3_384, url, user, s, w, resume, pbar) | ||
| + if downloadInfo.Size == 0 || resume < downloadInfo.Size { |
| - err = download(ctx, name, downloadInfo.Sha3_384, url, user, s, w, resume, pbar) | ||
| + if downloadInfo.Size == 0 || resume < downloadInfo.Size { | ||
| + err = download(ctx, name, downloadInfo.Sha3_384, url, user, s, w, resume, pbar) | ||
| + } else if downloadInfo.Sha3_384 != "" { |
pedronis
Aug 3, 2017
Contributor
this is weird, shouldn't this always be the case ? and if not, shouldn't we error earlier?
pedronis
Aug 3, 2017
Contributor
yes, maybe, but you changed real runtime code, the old code was always downloading, something seems off now
pedronis
Aug 3, 2017
•
Edited 1 time
-
pedronis
Aug 3, 2017
Contributor
-
Aug 3, 2017
what I mean, if you think we cannot error, it seems more conservative to behave as before in the case == "", not do a random thing as with this code
chipaca
Aug 3, 2017
Member
gah, turns out the only test that didn't set it that mattered was my own. fixed.
chipaca
merged commit 81dc18a
into
snapcore:master
Aug 3, 2017
7 checks passed
chipaca
deleted the
chipaca:do-not-resume-completed-downloads
branch
Aug 3, 2017
added a commit
to mvo5/snappy
that referenced
this pull request
Aug 14, 2017
chipaca commentedAug 2, 2017
•
Edited 1 time
-
chipaca
Aug 2, 2017
Without this a .partial file that has been completely downloaded still
triggers a request to the server (which will then come back with a
416).
This lets you drop a .snap in /var/lib/snapd/snaps as a .partial,
which could be used to speed up tests. It'll still be checksummed
(twice), but it won't hit the network for the download unless the
store says it's got a delta.
Doing the same thing for delta downloads is left as an exercise to the
reader.