Skip to content

Rework HTTP downloader retry logic #865

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.

Sign up for GitHub

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

Jump to bottom
Merged
smira merged 1 commit into from
Aug 7, 2019
Merged

Rework HTTP downloader retry logic #865

smira merged 1 commit into from
Aug 7, 2019

Conversation

@smira
Copy link
Contributor

@smira smira commented Aug 5, 2019 •
edited
Loading

Apply retries as global, config-level option downloadRetries so that
it can be applied to any aptly command which downloads objects.

Unwrap errors.Wrap which is used in downloader.

Unwrap *url.Error which should be the actual error returned from the
HTTP client, catch more cases, be more specific around failures.

@smira smira requested a review from a team August 5, 2019 21:45
@smira smira force-pushed the update-retry-logic branch from e9198ed to e954759 Compare August 5, 2019 22:05
@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #865 into master will decrease coverage by 0.05%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
- Coverage   64.02%   63.97%   -0.06%     
==========================================
  Files          51       51              
  Lines        6533     6540       +7     
==========================================
+ Hits         4183     4184       +1     
- Misses       1843     1848       +5     
- Partials      507      508       +1
Impacted Files Coverage Δ
http/download.go 73.01% <20%> (-3.46%) ⬇️

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 2e7f624...e954759. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 5, 2019 •
edited
Loading

Codecov Report

Merging #865 into master will increase coverage by 0.04%.
The diff coverage is 67.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
+ Coverage   64.02%   64.07%   +0.04%     
==========================================
  Files          51       51              
  Lines        6533     6556      +23     
==========================================
+ Hits         4183     4201      +18     
- Misses       1843     1850       +7     
+ Partials      507      505       -2
Impacted Files Coverage Δ
utils/config.go 64.7% <ø> (ø) ⬆️
context/context.go 11.28% <0%> (-0.22%) ⬇️
http/temp.go 77.77% <100%> (ø) ⬆️
http/compression.go 86.11% <100%> (ø) ⬆️
deb/remote.go 61.65% <100%> (ø) ⬆️
http/fake.go 62.29% <50%> (ø) ⬆️
http/download.go 80.14% <80.95%> (+3.67%) ⬆️

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 2e7f624...5e56ac6. Read the comment docs.

@smira
Copy link
Contributor Author

smira commented Aug 6, 2019

Travis issue discussion: https://travis-ci.community/t/travis-network-timeout-issue/4584

@smira
Copy link
Contributor Author

smira commented Aug 6, 2019

Tests are still failing which I believe is related to the Travis issue, but while at it, I refactored download retries, and now they can be enabled via config.

@smira smira added the 1.5.0 label Aug 6, 2019
smira
smira commented Aug 6, 2019
View reviewed changes
man/Makefile
@@ -1,10 +1,12 @@
all: prepare generate
RUBYBINPATH=$(shell ruby -r rubygems -e 'puts Gem.user_dir')/bin
Copy link
Contributor Author

@smira smira Aug 6, 2019

Choose a reason for hiding this comment

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

with that, make man runs under regular user account without access to system Ruby lib directories

@smira
Rework HTTP downloader retry logic
5e56ac6 
Apply retries as global, config-level option `downloadRetries` so that
it can be applied to any aptly command which downloads objects.

Unwrap `errors.Wrap` which is used in downloader.

Unwrap `*url.Error` which should be the actual error returned from the
HTTP client, catch more cases, be more specific around failures.
@smira smira force-pushed the update-retry-logic branch from 03c24ec to 5e56ac6 Compare August 7, 2019 17:07
@smira smira changed the title Update retry logic in HTTP downloader Rework HTTP downloader retry logic Aug 7, 2019
@smira
Copy link
Contributor Author

smira commented Aug 7, 2019

Merging this to unblock other PRs, and this also makes retries work much more reliably.

@smira smira merged commit f0a370d into aptly-dev:master Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@smira