Implement multipart upload for azureblob-sdk provider#904
Conversation
|
FYI @gaul I'm running this in a stage k8s cluster and it seems to work just fine. |
gaul
left a comment
There was a problem hiding this comment.
Please address comments in uploadMultipartPart but overall this looks good!
| } | ||
| int chunkSize = (int) Math.min(4 * 1024 * 1024L, | ||
| contentLength - position); | ||
| ByteBuffer buffer = ByteBuffer.allocate(chunkSize); |
There was a problem hiding this comment.
Can you allocate this outside the loop and reuse it within the loop? You will want to call ByteBuffer.reset here.
There was a problem hiding this comment.
this may be a bad idea because there is no guarantee that the AzureSDK strictly processes one ByteBuffer after another. Instead, we may overwrite a ByteBuffer that the AzureSDK still relies on.
|
@gaul addressed all your other comments. Thanks for reviewing it. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@DzeCin what makes you think so? There should be no state persisted in the s3 proxy. |
You are right I misread the code, sorry for the confusion. |
|
Thank you for your contribution @klaudworks! Let me tidy up a few other things but I will run a new release soon. |
|
@gaul no hurry, I am currently running a custom build in production anyways. Thank you for reviewing the changes! |
Current Approach
The current approach forwards the input stream from the incoming request to the azure sdk. I used a
Flux<ByteBuffer>to work around the limitation that our input stream does not support mark / reset. I looked into the azure-sdk-for-java after seeing your issue: Azure/azure-sdk-for-java#42603. It seems like this is the only workaround.Current Limitations
We don't handle if-match, if-none-match headers for conditional writes. This is supported by S3 for CompleteMultipartUpload requests.EDIT: I added a commit to support conditional writes for CompleteMultipartUpload requests.Alternative Approaches
Using a BufferedInputStream
I discarded this solution due to potentially high memory usage when uploading multiple parts in parallel.
Persisting the input stream on disk
Limitation 1: Potentially doubles the time until the part upload completes -> Can be compensated by uploading more files in parallel.
Limitation 2: Need to inform users that they provide sufficient /tmp storage on sufficiently fast SSDs. On an HDD the user would be heavily IO bound.
Benefit: We could actually calculate proper Etags for each part and store the md5 hash, e.g. encoded in the block name. This is not possible with the current solution because we need to already provide the block name when we pass along the input stream from the request to the azure sdk.
s3-tests Update
gaul/s3-tests#4
EDIT: I also running this on a test kubernetes cluster. It seems to work just fine. Some tooling (cnpg, kafka connector) can run their backups through it.
Fixes
#709 #553 #552