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.

Sign up for GitHub

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

Jump to bottom

Make S3 transfer operations cancellable #177

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Make S3 transfer operations cancellable #177

wants to merge 1 commit into from

Conversation

tschoonj
Copy link
Contributor

@tschoonj tschoonj commented Mar 8, 2021

Depends on boto/s3transfer#179

@tschoonj tschoonj changed the title S3Uploader: make transfer cancellable Make S3 transfer operations cancellable Mar 8, 2021
LAShemilt
LAShemilt reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@LAShemilt LAShemilt left a comment

Choose a reason for hiding this comment

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

Yes this is fine for me. Happy for it to be merged.

I made one comment about the use_thread=False option, I am not sure if this would cause the file monitor to hang on a really large file, but if you have tested this please go ahead and merge.

rfi_file_monitor/utils/s3.py

KB = 1024
MB = KB * KB
TransferConfig = boto3.s3.transfer.TransferConfig(max_concurrency=1, multipart_chunksize=8*MB, multipart_threshold=8*MB)
TransferConfig = boto3.s3.transfer.TransferConfig(max_concurrency=1, multipart_chunksize=8*MB, multipart_threshold=8*MB, use_threads=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this impact large files at all? Other than making them slower to upload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, let me quickly try that. I put this option in as it will be necessary for making the operations cancellable when that boto/s3transfer PR is merged in.

@tschoonj
Copy link
Contributor Author

This is very much WIP btw as it depends on fixes in s3transfer, and I will probably need to open a PR in boto3 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LAShemilt LAShemilt LAShemilt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tschoonj @LAShemilt