Skip to content
This repository was archived by the owner on Jul 13, 2023. It is now read-only.

Add fog_options to configuration that can be passed to the fog #2135

Merged
merged 1 commit into from
Mar 23, 2016

Conversation

jeremywadsack
Copy link
Contributor

We found that uploading large files to S3 would result in a socket error ("connection reset by peer") occasionally and lately much more consistently. In researching this I saw that many people got this error when uploading too large of a file without multipart chunking. I would have assumed fog did this automatically but the default chunk size may be too high. In order to address this I wanted to drop the chunk size to 100MB.

Rather than hard-code this I opted to expose a fog_option configuration option that lets me pass any additional options I want to the fog's #create call. This is similar to the fog_attributes option implemented in CarrierWave which addresses the same problem.

We've been running this now for a week in production and it seems to resolve the issue.

it "applies the options to the fog #create call" do
files = stub
@dummy.avatar.stubs(:directory).returns stub(files: files)
files.expects(:create).with(has_entries(:multipart_chunk_size => 104857600))

Choose a reason for hiding this comment

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

Line is too long. [86/80]
Use the new Ruby 1.9 hash syntax.

@tute
Copy link
Contributor

tute commented Mar 23, 2016

This is great, thank you @jeremywadsack! Can you please squash your commits together? Then I'll merge. Thank you!

@tute
Copy link
Contributor

tute commented Mar 23, 2016

Also, can you please add a note in the NEWS file? Thanks!

@jeremywadsack
Copy link
Contributor Author

@tute: Done.

Note that branched from 4.3.5 because I couldn't get master to store any files (it appeared broken). So I opened this PR against v4.3; however, it looks like v4.3 has now been updated (4.3.6?) and GitHub is saying that it's got conflicts. Do you want me to figure that out and verify this or are you going to handle the merge conflicts?

@tute
Copy link
Contributor

tute commented Mar 23, 2016

Please do rebase against latest commit in that branch and we merge. Thanks!

On Wednesday, March 23, 2016, Jeremy Wadsack notifications@github.com
wrote:

@tute https://github.com/tute: Done.

Note that branched from 4.3.5 because I couldn't get master to store any
files (it appeared broken). So I opened this PR against v4.3; however, it
looks like v4.3 has now been updated (4.3.6?) and GitHub is saying that
it's got conflicts. Do you want me to figure that out and verify this or
are you going to handle the merge conflicts?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2135 (comment)

@jeremywadsack
Copy link
Contributor Author

@tute all ready for you.

@tute tute merged commit 87e5ef5 into thoughtbot:v4.3 Mar 23, 2016
@tute
Copy link
Contributor

tute commented Mar 23, 2016

Thank you! :)

@tute
Copy link
Contributor

tute commented Mar 23, 2016

Will cherry-pick your commit into master, for v5 as well.

tute pushed a commit that referenced this pull request Mar 23, 2016
We found that uploading large files to S3 would result in a socket error
("connection reset by peer") occasionally and lately much more
consistently. In researching this I saw that many people got this error
when uploading too large of a file without multipart chunking. I would
have assumed fog did this automatically but the default chunk size may
be too high. In order to address this I wanted to drop the chunk size to
100MB.

Rather than hard-code this I opted to expose a `fog_option`
configuration option that lets me pass any additional options I want to
the fog's `#create` call. This is similar to the `fog_attributes` option
implemented in CarrierWave which [addresses the same
problem](http://stackoverflow.com/a/11867978/201911).

We've been running this now for a week in production and it seems to
resolve the issue.

#2135
@tute
Copy link
Contributor

tute commented Mar 23, 2016

I put in the master commit as commit message your PR description, which is quite great. Forgot to ask for that in v4.3 branch.
Thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants