Support multipart for lakectl presigned uploads#9283
Conversation
itaigilo
added
include-changelog
|
Also closes product#791. |
N-o-Z
left a comment
N-o-Z
left a comment
There was a problem hiding this comment.
It's very neat how everything comes together.
Please see my comment - it's a very quick fix.
In Addition - I think a clarification is required in upload.go:147
We don't really check if content implements ReaderAt but rather we pass a nil value which is then evaluated when determining whether to use multipart upload or not. I would revise that comment
| file io.Seeker | ||
| reader io.Reader | ||
| readerAt io.ReaderAt |
There was a problem hiding this comment.
There's no need for another field.
Define a new interface called readerSeeker that implements both io.Seeker and io.Reader and then use that type for the file parameter.
|
Thanks @N-o-Z for the review. I've also updated the comment (L:147) - it's confusing indeed. PTAL again 🙏 |
N-o-Z
left a comment
N-o-Z
left a comment
There was a problem hiding this comment.
LGTM,
Please open an issue to test this scenario in esti - lets make sure we do perform MPU on files > 5G
|
Added an Issue for Esti, |
2 participants
Closes #9015.
Change Description
Background
When using lakectl local, uploading with pre-signed doesn't utilizes multipart uploads for large files.
Bug Fix
Root cause
Very technical issue:
The
fileWrapperdefined inprogress.godoesn't implement aReadAt()func, hence doesn't implementio.ReaderAt.Since
presignUpload.Upload()requiresio.ReaderAtfor pre-signed, it checks if the fileWrapper (initialized atSyncManager.upload()) satisfies this, and since it doesn't, it doesn't use multipart for the upload flow.Solution
Implement
ReadAt()forfileWrapper, so thatSyncManager.upload()ends up using multipart if needed.Testing Details
Tested manually:
With or without pre-signed, and with or without multipart.