Add retry option to download from remote url #2577
Conversation
tashirosota
force-pushed
the
add_retry_option_to_download_from_remote_url
branch
2 times, most recently
from
8184134 to
9551486
Compare
lib/carrierwave/mounter.rb
Outdated
| retry | ||
| else | ||
| raise e | ||
| end |
There was a problem hiding this comment.
Please do not expose a download-specific logic outside of the downloader.
This should be moved to somewhere in CarrierWave::Downloader::Base.
There was a problem hiding this comment.
Hi @mshibuya!
Thanks for review, and many thanks for carrierwave！
I fixed for move to CarrierWave::Downloader::Base.
There was a problem hiding this comment.
@mshibuya
Hi, our application require this option.
Is it possible to merge this PR?
tashirosota
force-pushed
the
add_retry_option_to_download_from_remote_url
branch
from
9551486 to
62ee7e2
Compare
tashirosota
force-pushed
the
add_retry_option_to_download_from_remote_url
branch
from
62ee7e2 to
2f03617
Compare
lib/carrierwave/downloader/base.rb
Outdated
| # [remote_headers (Hash)] Request headers | ||
| # | ||
| def download(url, remote_headers = {}) | ||
| def download(url, download_retry_count = 0, remote_headers = {}) |
There was a problem hiding this comment.
Please don't insert download_retry_count into arguments, this will break existing implementations.
A downloader instance have a reference to an uploader instance, you can just reference uploader.download_retry_count inside the downloader.
spec/downloader/base_spec.rb
Outdated
| rescue CarrierWave::DownloadError | ||
| expect(instance.current_download_retry_count).to eq 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
Testing a certain behavior using its internal state (current_download_retry_count) is not a good idea. Instead, you can use this API to imitate download attempts which fails initially but successes in the next time.
https://github.com/bblimke/webmock#multiple-responses-for-repeated-requests
spec/downloader/base_spec.rb
Outdated
| let(:download_retry_count) { 0 } | ||
| it { expect { subject }.to raise_error CarrierWave::DownloadError } | ||
|
|
||
| it do |
There was a problem hiding this comment.
Don't omit the example name (it '...'), unless it can be read naturally using the one-liner syntax.
https://relishapp.com/rspec/rspec-core/docs/subject/one-liner-syntax
tashirosota
force-pushed
the
add_retry_option_to_download_from_remote_url
branch
3 times, most recently
from
c6744ad to
aa6a685
Compare
|
@mshibuya |
tashirosota
force-pushed
the
add_retry_option_to_download_from_remote_url
branch
from
aa6a685 to
d15c342
Compare
|
Awesome, thanks! |
Our project downloads image from imgix by use
remote_image_url, but imgix occasionally returns a 503 error and download failed.So I want to support to retry option in carrierwave.
This option will also be useful for other CDNs or storage.
Source: https://aws.amazon.com/premiumsupport/knowledge-center/s3-resolve-503-slowdown-throttling/?nc1=h_ls