api, core: Add new Core API {Get,Stat}Object with pre-conditions. #670

harshavardhana · 2017-04-26T07:49:23Z

Implements a new API to provide a way to set headers
for GetObject(), StatObject() request such as to

read partial data starting at offsets.
read only if etag matches.
read only if modtime matches.
read only if etag doesn't match.
read only if modtime doesn't match.

Fixes #669

krishnasrinivas · 2017-04-26T08:11:57Z

api-get-conditions.go

+// http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectGET.html
+type GetConditions struct {
+	http.Header
+}


why not:
type PreConditions http.Header

Too generic we already have CopyConditions.

But PreConditions are generic header information ... i.e it can apply to HEAD too if required.

SetRange does not sound right for a "Conditions" type.

You could name it just Header

Ah you are right.

SetRange is appropriate as it is part of the conditions here http://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectGET.html - Header is not conveying any information.

SetRange is under Request Headers section not conditions. i.e Preconditions belongs to Request Headers but Range does not belong to Preconditions

Range also is part of a pre-condition such that you need to validate the requested range first. So i don't agree.

From a HTTP point of view you are right Range is not part of Pre-Conditions - https://tools.ietf.org/html/rfc7232#section-3 one needs to set If-Range. But AWS S3 doesn't support that yet.

krishnasrinivas · 2017-04-26T08:20:34Z

api-get-object-file.go

+	// Initialize get object conditions and set the appropriate
+	// range offsets to read from.
+	conds := GetConditions{}
+	conds.SetRange(st.Size(), 0)


will panic as the map is nil

Ah will change this.

vadmeste · 2017-04-26T11:42:45Z

pre-conditions.go

+// SetRange - set the start and end offset of the object to be read.
+// See https://tools.ietf.org/html/rfc7233#section-3.1 for reference.
+func (c PreConditions) SetRange(start, end int64) error {
+	if start < 0 || end > 0 && end < start {


Can you reject when end is negative, it looks it can pass when end = -1 and start = 1 for example.

End negative is a valid case.. though.

vadmeste · 2017-04-26T11:47:30Z

api-get-object-partial.go

@@ -0,0 +1,17 @@
+package minio


Looks like this file is not needed.

will remove.

vadmeste · 2017-04-26T11:50:30Z

api-get-object-file.go

+	// Initialize get object conditions and set the appropriate
+	// range offsets to read from.
+	conds := NewPreConditions()
+	conds.SetRange(st.Size(), 0)


~~This should be conds.SetRange(0, st.Size()-1)~~

I understand the meaning of end = 0 now.

vadmeste · 2017-04-26T12:01:28Z

pre-conditions.go

+
+// SetRange - set the start and end offset of the object to be read.
+// See https://tools.ietf.org/html/rfc7233#section-3.1 for reference.
+func (c PreConditions) SetRange(start, end int64) error {


We can't download only the first byte with this primitive.

SetRange(0, 0) should be able to return the first byte.

I think end should be -1 when we need to indicate that we want to download from start until the end of the target object.

Yes there are more cases in pure HTTP, But all cases are not supported by AWS S3.

harshavardhana · 2017-04-26T19:07:03Z

Can you check again @krishnasrinivas @vadmeste

vadmeste · 2017-04-26T19:19:44Z

request-headers.go

+	case start > 0 && end > 0 && end > start:
+		// Read everything starting at 'start' till the 'end'. `bytes=N-M`
+		c.Set("Range", fmt.Sprintf("bytes=%d-%d", start, end))
+	case start == 0 && end == 0:


Only the first byte?

I guess S3 can support reading one byte in other positions. You can merge this case with the previous one, so it would be: case start > 0 && end > 0 && end >= start:

vadmeste

One comment

krisis · 2017-04-27T14:05:08Z

core_test.go

+	io.ReadFull(reader, buf3)
+	reader.Close()
+
+	if objectInfo.Size != int64(len(buf3)) {


The above check doesn't guarantee that len(buf3) bytes were read from reader. The check should be,

n, err = io.ReadFull(reader, buf3) if err != nil { t.Fatal(err) } if objectInfo.Size != n { t.Fatal(err) }

Why do you think that ?

Number of bytes read would be different than len(buf3) if io.ReadFull returned with io.UnexpectedEOF.

krisis · 2017-04-27T14:17:28Z

api-get-object-file.go

+	// Initialize get object conditions and set the appropriate
+	// range offsets to read from.
+	reqHeaders := NewGetReqHeaders()
+	reqHeaders.SetRange(st.Size(), 0)


As per, case start > 0 && end == 0: in SetRange(...) method this reqHeaders will request bytes of object starting from st.Size() to EOF, which is not intended. Shouldn't it be SetRange(0, -st.Size())?

I don't think so..

@harshavardhana which case statement would match SetRange(st.Size(), 0), where st.Size() > 0?

@krisis it looks confusing but st.Size() is actually size of the partially downloaded file, not the size of the object

Implements a new API to provide a way to set headers for GetObject(), StatObject() request such as to - read partial data starting at offsets. - read only if etag matches. - read only if modtime matches. - read only if etag doesn't match. - read only if modtime doesn't match. Fixes minio#669

harshavardhana requested review from krisis, krishnasrinivas and vadmeste April 26, 2017 07:49

harshavardhana force-pushed the get-object branch from b82a6f5 to 645600a Compare April 26, 2017 07:57

krishnasrinivas reviewed Apr 26, 2017

View reviewed changes

harshavardhana force-pushed the get-object branch 3 times, most recently from e4e3603 to 6245b83 Compare April 26, 2017 09:13

vadmeste reviewed Apr 26, 2017

View reviewed changes

harshavardhana force-pushed the get-object branch 7 times, most recently from bad0b1b to e69012b Compare April 26, 2017 19:06

harshavardhana changed the title ~~api, core: Introduce a new Core API with GetObject pre-conditions.~~ api, core: Add new Core API {Get,Stat}Object with pre-conditions. Apr 26, 2017

vadmeste reviewed Apr 26, 2017

View reviewed changes

vadmeste requested changes Apr 26, 2017

View reviewed changes

harshavardhana force-pushed the get-object branch 3 times, most recently from 86bdf05 to a6f206c Compare April 26, 2017 22:34

vadmeste approved these changes Apr 27, 2017

View reviewed changes

krisis requested changes Apr 27, 2017

View reviewed changes

harshavardhana force-pushed the get-object branch from a6f206c to 5bf96ba Compare April 27, 2017 17:43

krishnasrinivas approved these changes Apr 27, 2017

View reviewed changes

harshavardhana force-pushed the get-object branch from 5bf96ba to d09fe2a Compare April 28, 2017 01:27

krisis approved these changes Apr 28, 2017

View reviewed changes

harshavardhana force-pushed the get-object branch 3 times, most recently from 982ad47 to 3aa4ee3 Compare April 28, 2017 08:01

harshavardhana force-pushed the get-object branch from 3aa4ee3 to c6a7758 Compare April 28, 2017 08:11

harshavardhana merged commit 44db87a into minio:master Apr 28, 2017

harshavardhana deleted the get-object branch April 28, 2017 08:38

harshavardhana mentioned this pull request Apr 28, 2017

Validate minio.Object ETag between read requests #662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api, core: Add new Core API {Get,Stat}Object with pre-conditions. #670

api, core: Add new Core API {Get,Stat}Object with pre-conditions. #670

harshavardhana commented Apr 26, 2017 •

edited

Loading

krishnasrinivas Apr 26, 2017

harshavardhana Apr 26, 2017

krishnasrinivas Apr 26, 2017

harshavardhana Apr 26, 2017

harshavardhana Apr 26, 2017

krishnasrinivas Apr 26, 2017

harshavardhana Apr 26, 2017

harshavardhana Apr 26, 2017

krishnasrinivas Apr 26, 2017

harshavardhana Apr 26, 2017

vadmeste Apr 26, 2017

harshavardhana Apr 26, 2017

vadmeste Apr 26, 2017

harshavardhana Apr 26, 2017

vadmeste Apr 26, 2017 •

edited

Loading

vadmeste Apr 26, 2017

harshavardhana Apr 26, 2017

harshavardhana commented Apr 26, 2017

vadmeste Apr 26, 2017

harshavardhana Apr 26, 2017

vadmeste left a comment

krisis Apr 27, 2017

harshavardhana Apr 27, 2017

krisis Apr 27, 2017

krisis Apr 27, 2017

harshavardhana Apr 27, 2017

krisis Apr 27, 2017

krishnasrinivas Apr 27, 2017

api, core: Add new Core API {Get,Stat}Object with pre-conditions. #670

api, core: Add new Core API {Get,Stat}Object with pre-conditions. #670

Conversation

harshavardhana commented Apr 26, 2017 • edited Loading

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

Choose a reason for hiding this comment

vadmeste Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadmeste 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana commented Apr 26, 2017 •

edited

Loading

vadmeste Apr 26, 2017 •

edited

Loading