UploadStreamToBlockBlob gets TransferManager option #234
+254
−91
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See: Azure#233 There needs to be a more robust way of controling memory reuse and threadpooling for streaming copies (or maybe all copies, I will not speak to that here). This introduces a new interface called TransferManager. This provides 3 main fuctions: - Control buffer allocation - Control goroutine allocation - Allow reuse This allows a developer to control exactly how they wish to handle this for their use case. We provide two implementations: - Static buffer allocation similar to the original implementation - Threadpool allocation similar to how it currently works since my change Additionally, both of these support having shared pools between calls which neither did before. It also allows for custom creation of implementations by users that better support their needs (a hybrid of the two above, one that adjusts buffer or concurrency based on transfer speed, ...) Note: the behavior by default returns to the static buffer method here. If azcopy likes the current behavior, it should switch using NewSyncPool() Like the original change, this does not change the API compatibility. I think that is probably something that should be done, but not in this change. There are 3 files here that were not part of my change, but were not go fmt, so they have been updated by the go tool: - bytes_wrtier.go - zc_* All tests without the end to end test system that are expected to pass do (comparing the non-changed version against this one).
|
Please see conversation here: |
adreed-msft
reviewed
Jan 26, 2021
| } | ||
|
|
||
| threadpool := make(chan func(), max) | ||
| buffers := make(chan []byte, max) |
There was a problem hiding this comment.
Not sure how I feel about threadpooled anonymous functions having no access to the buffer which hosts them.
Also not sure how I feel about the threadpool itself being attached to the buffer, since it's not really used that way... How do you feel about separating these out?
|
I feel like in comparison to the other PR available, this one is much more user-friendly, which would be the vibe we're aiming for more. |
adreed-msft
approved these changes
Jan 26, 2021
zezha-msft
approved these changes
Jan 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See: #233
There needs to be a more robust way of controling memory reuse and threadpooling for streaming copies (or maybe all copies, I will not speak to that here).
This introduces a new interface called TransferManager. This provides 3 main fuctions:
This allows a developer to control exactly how they wish to handle this for their use case. We provide two implementations:
Additionally, both of these support having shared pools between calls which is new.
It also allows for custom creation of implementations by users that better support their needs (a hybrid of the two above, one that adjusts buffer or concurrency based on transfer speed, ...)
Note: the behavior by default returns to the static buffer method here. If azcopy likes the current behavior, it should switch using NewSyncPool()
Like the original change, this does not change the API compatibility. I think that is probably something that should be done, but not in this change.
There are 3 files here that were not part of my change, but were not go fmt'd, so they have been updated by the go tool:
All tests without the end to end test system that are expected to pass do (comparing the failed tests in the non-changed version against this one).