Add large-object support for CopyObject API #644

donatello · 2017-04-10T12:29:24Z

This patch is up for early review.

Context of the change is at #617

The byte-range for the source object has been added into CopyConditions type - ideally the CopyConditions type should be called CopySourceInfo and it should have the copy-conditions, copy-source and the optional copy-byte-range. However, the changes in this patch aim to keep backwards compatibility.

The one wrinkle is the NewCopyConditions method - it is now the preferred method to create a CopyConditions{} instance as it initializes the byte-range to -1 (indicating that the byte range is not specified by the user).

This is as yet untested - hoping to get some input about the interface improvement to be done.

donatello · 2017-04-11T07:58:04Z

API change proposal:

func CopyObject(bucketName, objectName, srcBucket, srcObject string, csi copySourceInfo) error

Providing explicit source bucket and object makes it easier for user to not have to format it as "/bucket/object".

Data type and methods:

// Not exported so that user has to use an initialization function
type copySourceInfo struct {
    copyConditions map[string]string
    byteRangeStart int64
    byteRangeEnd int64
}

// To allocate map and initialize range
func NewCopySourceInfo() copySourceInfo {}

// funcs to set copy conditions.
func (c *copySourceInfo) SetMatchETag(etag string) error {}
func (c *copySourceInfo) SetMatchETagExcept(etag string) error {}
func (c *copySourceInfo) SetUnmodified(modTime time.Time) error {}
func (c *copySourceInfo) SetModified(modTime time.Time) error {}
func (c *copySourceInfo) SetByteRange(start, end int64) error {}

// internal utils
func (c *copySourceInfo) getRangeSize() int64 {}
func (c *copySourceInfo) duplicate() *copySourceInfo {}

harshavardhana · 2017-04-12T21:40:01Z

To keep backward compatibility we can introduce a new API, we now have projects dependent on minio-go. So we need to be careful with API changes.

Will discuss this with @abperiasamy and see where we go from there.

harshavardhana · 2017-04-12T21:41:28Z

copy-conditions.go

+	return c.byteRangeEnd - c.byteRangeStart + 1
+}
+
+func (c *CopyConditions) duplicate() *CopyConditions {


call this clone() or cloneCopyConditions in Go style.

harshavardhana · 2017-04-12T21:43:09Z

api-put-object-copy.go

+	}
+	return parts[1], parts[2], nil
+}
+


Comment necessary..

harshavardhana · 2017-04-12T21:43:16Z

api-put-object-copy.go

@@ -70,3 +102,91 @@ func (c Client) CopyObject(bucketName string, objectName string, objectSource st
 	// Return nil on success.
 	return nil
 }
+
+func getObjectSource(src string) (bucket string, object string, err error) {


Comment necessary.. and a test case..

krisis

Minor comments, overall looks good.

krisis · 2017-04-15T05:46:17Z

api-put-object-copy.go

+	if err != nil {
+		return err
+	}
+


Stray newline?

krisis · 2017-04-15T05:48:13Z

api-put-object-copy.go

+			cpCond.byteRangeStart, cpCond.byteRangeEnd, srcInfo.Size))
+	}
+
+	copySize := srcByteRangeSize


A comment explaining how an empty range is interpreted as the entire object would provide context for future.

vadmeste · 2017-04-15T10:34:00Z

copy-conditions.go

+func (c *CopyConditions) getRangeSize() int64 {
+	if c.byteRangeStart < 0 {
+		// only happens if byte-range was not set by user
+		return 0


Probably it is better to set this to -1 to say unknown. Probably zero is a valid value.

vadmeste · 2017-04-15T10:47:01Z

copy-conditions.go

+	if start < 0 || end < start {
+		return ErrInvalidArgument("Range start less than 0 or range end less than range start.")
+	}
+	if end-start+1 < 1 {


I think this condition is useless,

Here we are already sure that start <= end:

start <= end <=> 0 <= end - start <=> 1 <= end - start + 1,

so we will never have end-start+1 < 1

harshavardhana · 2017-04-26T07:18:53Z

api-put-object-copy.go

+
+		// Update the source range header value.
+		headers.Set("x-amz-copy-source-range",
+			fmt.Sprintf("bytes:%d-%d", pCond.byteRangeStart,


This is wrong btw - it should be bytes=%d-%d

deekoder · 2017-05-12T22:16:16Z

Is this still blocked?

harshavardhana · 2017-05-12T22:25:27Z

Is this still blocked?

Yes not discussed yet.

harshavardhana · 2017-05-22T19:59:26Z

@donatello can you update this and incorporate usage of requestHeaders instead of copyConditions . Which would expand the scope of this.

harshavardhana · 2017-06-02T18:39:35Z

@donatello can you update this and incorporate usage of requestHeaders instead of copyConditions . Which would expand the scope of this.

@donatello can you incorporate the request-headers.go changes to CopyObject() just like how we did with GetObject() .This would be a breaking change we will bump up the tag so that the users who have referenced using our tags atleast can understand why this change.

tejaycar · 2017-06-05T18:58:11Z

@donatello, @harshavardhana is there anything I can do to help?

donatello · 2017-06-08T19:02:27Z

@tejaycar We plan to get this resolved by next week. We are busy with the Minio server release at this time.

tejaycar · 2017-06-08T21:27:44Z

Awesome. Thanks guys.

…

On Jun 8, 2017 1:02 PM, "Aditya Manthramurthy" ***@***.***> wrote: @tejaycar <https://github.com/tejaycar> We plan to get this resolved by next week. We are busy with the Minio server release at this time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADqwzK9gCNi5cUj-jujxrK654Mrs0oQOks5sCEVEgaJpZM4M4tp5> .

tejaycar · 2017-06-19T18:48:34Z

@donatello I don't mean to bug, but do you have any update to offer on this? I'm not trying to push, just trying to plan accordingly.

donatello · 2017-06-19T19:32:56Z

@donatello I don't mean to bug, but do you have any update to offer on this? I'm not trying to push, just trying to plan accordingly.

@tejaycar I am actively working on this, and hope to have a PR today. I would venture to say that it might get merged by the end of the week after reviews and testing.

tejaycar · 2017-06-19T19:34:38Z

Once again, thank you. I appreciate the update.

…

On Mon, Jun 19, 2017 at 1:32 PM, Aditya Manthramurthy < ***@***.***> wrote: @donatello <https://github.com/donatello> I don't mean to bug, but do you have any update to offer on this? I'm not trying to push, just trying to plan accordingly. @tejaycar <https://github.com/tejaycar> I am actively working on this, and hope to have a PR today. I would venture to say that it might get merged by the end of the week after reviews and testing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADqwzF7LnGfI3104fu0qsIiPOeVGdfTnks5sFszqgaJpZM4M4tp5> .

donatello · 2017-06-20T02:26:14Z

We are bringing large object copy support via a more flexible ComposeObject API that allows server-side copying of objects to create new objects.

This PR is thus superseded by #715. Closing as obsolete.

donatello force-pushed the copy-object branch from 651f3e8 to 8ee5c56 Compare April 10, 2017 12:30

Add large-object support for CopyObject API

0e50c93

donatello force-pushed the copy-object branch from 8ee5c56 to 0e50c93 Compare April 10, 2017 12:31

harshavardhana requested a review from krisis April 12, 2017 21:40

harshavardhana requested changes Apr 12, 2017

View reviewed changes

harshavardhana requested a review from vadmeste April 12, 2017 23:10

harshavardhana added the blocked label Apr 12, 2017

krisis reviewed Apr 15, 2017

View reviewed changes

vadmeste reviewed Apr 15, 2017

View reviewed changes

harshavardhana changed the title ~~WIP: Add large-object support for CopyObject API~~ Add large-object support for CopyObject API Apr 17, 2017

harshavardhana reviewed Apr 26, 2017

View reviewed changes

harshavardhana mentioned this pull request May 22, 2017

CopyObject Does not support metadata #690

Closed

fd0 mentioned this pull request Jun 11, 2017

PutObject() with unsizeable reader on restricted account only returns io.EOF? #702

Closed

donatello closed this Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add large-object support for CopyObject API #644

Add large-object support for CopyObject API #644

donatello commented Apr 10, 2017

donatello commented Apr 11, 2017

harshavardhana commented Apr 12, 2017

harshavardhana Apr 12, 2017

harshavardhana Apr 12, 2017

harshavardhana Apr 12, 2017

krisis left a comment

krisis Apr 15, 2017

krisis Apr 15, 2017

vadmeste Apr 15, 2017

vadmeste Apr 15, 2017

harshavardhana Apr 26, 2017

deekoder commented May 12, 2017

harshavardhana commented May 12, 2017

harshavardhana commented May 22, 2017

harshavardhana commented Jun 2, 2017

tejaycar commented Jun 5, 2017

donatello commented Jun 8, 2017

tejaycar commented Jun 8, 2017 via email

tejaycar commented Jun 19, 2017

donatello commented Jun 19, 2017

tejaycar commented Jun 19, 2017 via email

donatello commented Jun 20, 2017

Add large-object support for CopyObject API #644

Add large-object support for CopyObject API #644

Conversation

donatello commented Apr 10, 2017

donatello commented Apr 11, 2017

harshavardhana commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisis left a 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

deekoder commented May 12, 2017

harshavardhana commented May 12, 2017

harshavardhana commented May 22, 2017

harshavardhana commented Jun 2, 2017

tejaycar commented Jun 5, 2017

donatello commented Jun 8, 2017

tejaycar commented Jun 8, 2017 via email

tejaycar commented Jun 19, 2017

donatello commented Jun 19, 2017

tejaycar commented Jun 19, 2017 via email

donatello commented Jun 20, 2017