-
Notifications
You must be signed in to change notification settings - Fork 647
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
api: Optimize multipart upload memory usage for unknown sized stream #776
Conversation
Any reviews here @balamurugana @vadmeste ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use byte slice instead of converting it to writer?
api-put-object-encrypted.go
Outdated
@@ -42,5 +42,5 @@ func (c Client) PutEncryptedObject(bucketName, objectName string, reader io.Read | |||
metadata[amzHeaderKey] = []string{encryptMaterials.GetKey()} | |||
metadata[amzHeaderMatDesc] = []string{encryptMaterials.GetDesc()} | |||
|
|||
return c.putObjectMultipart(bucketName, objectName, encryptMaterials, -1, metadata, progress) | |||
return c.putObjectMultipartStreamNoLength(bucketName, objectName, encryptMaterials, -1, metadata, progress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is -1 to putObjectMultipartStreamNoLength()
?
Changes are done @balamurugana - ready to review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
api-put-object-multipart.go
Outdated
prtSize, rErr := hashCopyN(hashAlgos, hashSums, tmpBuffer, reader, partSize) | ||
if rErr != nil && rErr != io.EOF { | ||
bufp := bufPool.Get().(*[]byte) | ||
prtSize, rErr := io.ReadFull(reader, *bufp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
length
may be better than prtSize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
|
||
// Increment part number. | ||
partNumber++ | ||
|
||
// Put back data into bufpool. | ||
bufPool.Put(bufp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this statement can be in defer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot defer @balamurugana we need to get this back in the next loop.
@vadmeste need your reviews as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM & tested with functional tests
Fixes #730