Refactor PutObject API calls to add context and additional metadata. #811

poornas · 2017-09-05T20:41:30Z

Add additional Api calls PutObjectWithContext, GetObjectWithContext, FPutObjectWithContext and FGetObjectWithContext to allow request cancellation.
Remove PutObjectWithSize,PutObjectStreaming,PutEncryptedObject,PutObjectWithMetadata calls and fold into a single PutObject call which accepts a pointer to PutObjectOptions struct. PutObjectOptions struct allows user to enter custom metadata, Content-Encoding, Cache-Control, Content-Type and Content-Disposition headers, configure number of threads for multipart PutObject calls, and set Encryption materials module to encrypt data.
Change FPutObject API signature to add additional parameter - a pointer to PutObjectOptions struct.
Number of workers used in multipart Put operation is now configurable by setting the number of threads as an optional field in the PutObjectOptions struct.

Fixes #700
Fixes #747
Fixes #759

houndci-bot · 2017-09-05T20:41:43Z

api-put-object-context.go

+	"io"
+)
+
+func (c Client) PutObjectWithContext(ctx context.Context, bucketName, objectName string, reader io.Reader, objectSize int64,


exported method Client.PutObjectWithContext should have comment or be unexported

houndci-bot · 2017-09-05T20:41:43Z

api-put-object.go

 	"github.com/minio/minio-go/pkg/s3utils"
 )

+type PutObjectOptions struct {


exported type PutObjectOptions should have comment or be unexported

vadmeste · 2017-09-08T16:07:05Z

api-get-object-file.go

 	"github.com/minio/minio-go/pkg/s3utils"
 )

 // FGetObject - download contents of an object to a local file.
 func (c Client) FGetObject(bucketName, objectName, filePath string) error {
+	return c.fGetObjectWithContext(context.Background(), bucketName, objectName, filePath)


I am not sure how it is useful to provide two APIs which has the same functionality, for example here:

FGetObject
and

FGetObjectWithContext

I think we need to decide if they should have different behaviors or merge them into one function

It was decided to have an extra variant of Put/GetObject when context is an additional argument, rather than force user to pass nil each time if they do not need request cancellation. We could however revisit this in the next meeting and reach a consensus.

rather than force user to pass nil each time if they do not need request cancellation.

emm, in that case, we need to pass nil here (if it works) and not context.Background()

as per https://golang.org/pkg/context/
Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

Since we are wrapping the functions, it makes sense to use context.Background(). Agree?

Since we are wrapping the functions, it makes sense to use context.Background(). Agree?

sorry, yeah I see now how it can be useful to have two APIs

poornas · 2017-09-14T01:21:11Z

@vadmeste and @harshavardhana , this needs reviews.

harshavardhana · 2017-09-14T18:42:27Z

api-put-object-context.go

+	}
+
+	if opts.EncryptMaterials != nil {
+		if err := opts.EncryptMaterials.SetupEncryptMode(reader); err != nil {


This err assignment shadows with err return value. Just use err = instead of err :=

harshavardhana · 2017-09-14T18:44:01Z

api-put-object-file-context.go

+		opts = &PutObjectOptions{}
+	}
+	// Set contentType based on filepath extension if not given or default
+	// value of "binary/octet-stream" if the extension has no associated type.


Perhaps we can name this as application/octet-stream instead. Since that is what we set eventually instead of binary/octet-stream

this was unintentional. fixed.

harshavardhana · 2017-09-14T18:45:51Z

api-put-object.go

+
+// getMetaData - constructs the headers from metadata entered by user in
+// PutObjectOptions struct
+func (opts *PutObjectOptions) getMetadata() (headers map[string][]string) {


We can make the return value to be more typed i.e getMetadataHeaders() (headers http.Header) instead.

harshavardhana

This is a major change in API we need to move to 4.0 after this change is merged.

harshavardhana · 2017-09-15T10:04:20Z

One suggestion is regarding commit message.

Add additional Api calls PutObjectWithContext, GetObjectWithContext,
FPutObjectWithContext and FGetObjectWithContext to allow request cancellation.

Remove PutObjectWithSize,PutObjectStreaming,PutEncryptedObject,PutObjectWithMetadata
calls and fold into a single PutObject call which accepts a pointer to PutObjectOptions
struct. PutObjectOptions struct allows user to enter custom metadata, Content-Encoding,
Cache-Control, Content-Type and Content-Disposition headers, configure number of threads
for multipart PutObject calls, and set Encryption materials module to encrypt data.

^^ This paragraph can be worded a bit better.

Pass pointer to PutObjectOptions struct as argument to FPutObject call.

This is a not a commit body worthy message can can be removed.

Core API methods PutObject, NewMultipartUpload, PutObjectPart and PutObjectPartWithMetadata
signatures have changed.

This can be moved above along with function signature changes.

Remove go 1.5,1.6 from Travis. Add 1.9

We can say Deprecated support for Go versions 1.5.x, 1.6.x, Add support for 1.9.x

poornas · 2017-09-15T16:55:13Z

@harshavardhana, reworded commit message.

poornas · 2017-09-15T17:27:06Z

@vadmeste , this needs to be reviewed.

vadmeste · 2017-09-16T09:50:03Z

api-get-policy.go

@@ -79,7 +80,7 @@ func (c Client) getBucketPolicy(bucketName string) (policy.BucketAccessPolicy, e
 	urlValues.Set("policy", "")

 	// Execute GET on bucket to list objects.
-	resp, err := c.executeMethod("GET", requestMetadata{
+	resp, err := c.executeMethod(context.Background(), "GET", requestMetadata{


@poornas, is there any reason why we are not accepting letting developers their own context here?

@vadmeste, the scope of this PR is to limit adding context to PutObject calls because it is likely to be long running enough to warrant a request cancellation. There is not much of a use case for GetBucketPolicy to use context.

vadmeste · 2017-09-16T10:01:08Z

api-put-object-common.go

@@ -77,7 +78,7 @@ func optimalPartInfo(objectSize int64) (totalPartsCount int, partSize int64, las

 // getUploadID - fetch upload id if already present for an object name
 // or initiate a new request to fetch a new upload id.
-func (c Client) newUploadID(bucketName, objectName string, metaData map[string][]string) (uploadID string, err error) {
+func (c Client) newUploadID(ctx context.Context, bucketName, objectName string, opts *PutObjectOptions) (uploadID string, err error) {


I like PutObjectOptions change, but couldn't we do it in a separate PR ? (let's keep it as it now since I am already late reviewing this PR)

@vadmeste, the PutObjectOptions change is the main reason for this PR. Context change is minor since we are just passing it through to the executeMethod everywhere so that request can get context. It made sense to combine the changes in one PR.

vadmeste · 2017-09-16T10:51:32Z

api-get-object-context.go

+import "context"
+
+// GetObjectWithContext - returns an seekable, readable object.
+func (c Client) GetObjectWithContext(ctx context.Context, bucketName, objectName string) (*Object, error) {


I think we can avoid creating new file for this function, we can put it in api-get-file.go

vadmeste · 2017-09-16T10:52:17Z

api-get-object-file-context.go

+import "context"
+
+// FGetObjectWithContext - download contents of an object to a local file.
+func (c Client) FGetObjectWithContext(ctx context.Context, bucketName, objectName, filePath string) error {


We probably need to avoid creating a new file for this function

vadmeste · 2017-09-16T11:38:45Z

api-put-object.go

 			break
 		}
-		if rErr != nil && rErr != io.ErrUnexpectedEOF {
+		if rErr != nil && rErr != io.ErrUnexpectedEOF && partNumber > 1 {


Here, io.ReadFull() can return an error other than io.ErrUnexpectedEOF and io.EOF but we ignore it when partNumber == 1. In other terms, we will create an object even if the reader returns a serious error.

vadmeste · 2017-09-16T12:22:52Z

api-put-object-context.go

+			return 0, err
+		}
+
+		// Set the necessary encryption headers, for future decryption.


Can you remove this comment ? it doesn't make sense anymore

Add additional Api calls PutObjectWithContext, GetObjectWithContext, FPutObjectWithContext and FGetObjectWithContext to allow request cancellation. Remove PutObjectWithSize,PutObjectStreaming,PutEncryptedObject,PutObjectWithMetadata calls and fold into a single PutObject call which accepts a pointer to PutObjectOptions struct. PutObjectOptions struct allows user to optionally set custom metadata, content response headers, encryption materials module for data encryption, and specify number of threads for a multi-part Put operation. Currently,Content-Encoding,Cache-Control, Content-Type and Content-Disposition headers are supported. Deprecated support for Go versions 1.5.x,1.6.x. Add support for 1.9.x

poornas · 2017-09-19T18:19:20Z

@vadmeste, updated the PR with your suggestions.Could you please review?

vadmeste

LGTM, tested some examples + encryption example

houndci-bot reviewed Sep 5, 2017

View reviewed changes

poornas mentioned this pull request Sep 5, 2017

Api: Add PutObjectWithContext, GetObjectWithContext, FPutObjectWithCo… #765

Closed

poornas force-pushed the kfactor branch from 322f364 to aa9c021 Compare September 5, 2017 20:57

poornas mentioned this pull request Sep 6, 2017

Allow number of workers used for multipart operation to be configurable #794

Closed

poornas force-pushed the kfactor branch 3 times, most recently from 16b66cb to 903bf91 Compare September 6, 2017 23:20

deekoder requested review from vadmeste and harshavardhana September 8, 2017 01:37

vadmeste reviewed Sep 8, 2017

View reviewed changes

poornas force-pushed the kfactor branch 4 times, most recently from ac03556 to 10d9d29 Compare September 9, 2017 00:22

poornas mentioned this pull request Sep 12, 2017

Allow multipart threads to be configured via environment variable minio/mc#2241

Closed

poornas force-pushed the kfactor branch 5 times, most recently from 4211d64 to 705b148 Compare September 14, 2017 00:43

poornas force-pushed the kfactor branch 2 times, most recently from e305847 to ed85921 Compare September 14, 2017 17:31

harshavardhana requested changes Sep 14, 2017

View reviewed changes

poornas force-pushed the kfactor branch from ed85921 to 27cadb8 Compare September 14, 2017 18:57

harshavardhana approved these changes Sep 15, 2017

View reviewed changes

poornas force-pushed the kfactor branch from 27cadb8 to 9f3fc75 Compare September 15, 2017 16:54

poornas force-pushed the kfactor branch from 9f3fc75 to 5f601db Compare September 15, 2017 17:13

vadmeste reviewed Sep 16, 2017

View reviewed changes

poornas force-pushed the kfactor branch from 5f601db to b83685e Compare September 18, 2017 11:38

vadmeste approved these changes Sep 19, 2017

View reviewed changes

deekoder merged commit f98a667 into minio:master Sep 20, 2017

poornas mentioned this pull request Sep 20, 2017

add mint test for metadata size restrictions #816

Merged

poornas deleted the kfactor branch September 21, 2017 18:00

AGFeldman mentioned this pull request Nov 27, 2017

Fix minio PutObject facebookarchive/rocks-strata#36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor PutObject API calls to add context and additional metadata. #811

Refactor PutObject API calls to add context and additional metadata. #811

poornas commented Sep 5, 2017 •

edited by harshavardhana

houndci-bot Sep 5, 2017

houndci-bot Sep 5, 2017

vadmeste Sep 8, 2017

poornas Sep 8, 2017

vadmeste Sep 8, 2017

poornas Sep 8, 2017

vadmeste Sep 8, 2017 •

edited

poornas commented Sep 14, 2017

harshavardhana Sep 14, 2017

poornas Sep 14, 2017

harshavardhana Sep 14, 2017

poornas Sep 14, 2017

harshavardhana Sep 14, 2017

poornas Sep 14, 2017

harshavardhana left a comment

harshavardhana commented Sep 15, 2017 •

edited

poornas commented Sep 15, 2017

poornas commented Sep 15, 2017

vadmeste Sep 16, 2017

poornas Sep 17, 2017

vadmeste Sep 17, 2017

vadmeste Sep 16, 2017

poornas Sep 18, 2017

vadmeste Sep 16, 2017

poornas Sep 18, 2017

vadmeste Sep 16, 2017

poornas Sep 18, 2017

vadmeste Sep 16, 2017

poornas Sep 18, 2017

vadmeste Sep 16, 2017

poornas Sep 18, 2017

poornas commented Sep 19, 2017

vadmeste left a comment

Refactor PutObject API calls to add context and additional metadata. #811

Refactor PutObject API calls to add context and additional metadata. #811

Conversation

poornas commented Sep 5, 2017 • edited by harshavardhana

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadmeste Sep 8, 2017 • edited

Choose a reason for hiding this comment

poornas commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana left a comment

Choose a reason for hiding this comment

harshavardhana commented Sep 15, 2017 • edited

poornas commented Sep 15, 2017

poornas commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poornas commented Sep 19, 2017

vadmeste left a comment

Choose a reason for hiding this comment

poornas commented Sep 5, 2017 •

edited by harshavardhana

vadmeste Sep 8, 2017 •

edited

harshavardhana commented Sep 15, 2017 •

edited