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

Add a retry for downloading files via civis.io.file_to_civis #273

Merged
merged 1 commit into from
Dec 3, 2018

Conversation

mheilman
Copy link
Contributor

It also fixes a related bug in file_to_civis where the buffer was reset to 0 rather than the original position when retrying.

Fixes #272.

It also fixes a related bug in file_to_civis where the buffer was reset to 0 rather than the original position when retrying.
@mheilman
Copy link
Contributor Author

@waltaskew? @jkulzick?

@mheilman mheilman changed the title This adds a retry for downloading files via civis.io.file_to_civis. Add a retry for downloading files via civis.io.file_to_civis. Nov 30, 2018
@mheilman mheilman changed the title Add a retry for downloading files via civis.io.file_to_civis. Add a retry for downloading files via civis.io.file_to_civis Nov 30, 2018
Copy link
Contributor

@jkulzick jkulzick left a comment

Choose a reason for hiding this comment

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

LGTM
see my comment if you want to optimize

buf_orig_position = buf.tell()

@retry(RETRY_EXCEPTIONS)
def _download_url_to_buf():
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to do this without re-downloading the bytes you already retrieved is to use range headers in the HTTP Get request like so. You can probably do something similar on Put but I haven't done that before.

Up to you...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, but I'm going to punt on it for now. I filed #274.

@mheilman mheilman merged commit bdb197b into civisanalytics:master Dec 3, 2018
@mheilman mheilman deleted the s3_retry branch December 3, 2018 17:38
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.

Retry on Intermittent S3 Errors
2 participants