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

Davidslv/add stream for mixpanel response #45

Closed
wants to merge 3 commits into from
Closed

Davidslv/add stream for mixpanel response #45

wants to merge 3 commits into from

Conversation

Davidslv
Copy link

Hey @keolo, I've modified the Mixpanel::URI.get to handle stream instead of parsing and read the whole response, which can be quite big on an export, since it's per day, a project with many events/properties or events with many properties can take quite a while to "download" everything into memory. This should close #44

Thank you

@keolo
Copy link
Owner

keolo commented Jun 4, 2015

This seems like a great idea. This could use some specs and refactoring (e.g. try running rubocop) before merging.

@Davidslv
Copy link
Author

Davidslv commented Jun 19, 2015

Hey @keolo, I did it in a way that it didn't change the api, and still made your specs pass.
Although because of the latest, I think the implementation is not correct.
Meaning that now it downloads the file in chunks but then it still has to put it in a string variable, which will most likely cause memory issues. So I left your method alone and I created another one called download which accepts a directory and a filename so that all the writing is done in chunks to a specified file. You can check it out here: https://github.com/Superfling/mixpanel_client/blob/master/lib/mixpanel/uri.rb#L21 although we decided to not download the whole export for what we need, hence the file_size limit here ( https://github.com/Superfling/mixpanel_client/blob/master/lib/mixpanel/uri.rb#L30 ) but that can be changed easily if you decide to adopt it.

Thank you

@Davidslv Davidslv closed this Dec 4, 2017
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.

Streaming Response Bodies
2 participants