Skip to content
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

Added noDisk option that keeps each part's content in memory instead of in a temp file #6

Closed
wants to merge 4 commits into from

Conversation

aarsilv
Copy link

@aarsilv aarsilv commented Apr 6, 2013

Platform as a service offerings such as nodejitsu either forbid or limit disk writes. With the current implementation I was unable to large amounts of text (giant generated CSV files) to S3 as the application would throw errors.

I've added a noDisk option which when set to true keeps the data for each part in-memory instead of writing to a temporary file. It defaults to false.

Also added trivial change of including status code when an error is thrown because response code is not 200, as this was helpful for debugging.

@mikermcneil
Copy link
Collaborator

+1

This would solve the lingering ENOENT errors that pop up

@nathanoehlman
Copy link
Owner

Hi @Zugwalt - you make a good case for buffering the file into memory instead of the file - my initial version that I used did this, but I made a deliberate design decision to switch to files instead because I didn't want to have to put a bit of logic along the lines of what @Shahriman proposed at that stage in order to prevent people accidentally crashing their servers.

I intend on refactoring your commit a little in the future to use an in memory stream (so that the file and memory streams are easily interchangeable), as well as take into account @Shahriman's changes - but seeing as how I'm a little short strapped for time at the moment, I'll just pull yours in for the moment, and refactor later on.

Thanks :)

@aarsilv
Copy link
Author

aarsilv commented Jul 19, 2013

Awesome Nathan! Thanks!

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

Successfully merging this pull request may close these issues.

4 participants