PutObject streams with unknown size use streaming V4. #750

harshavardhana · 2017-07-10T05:10:43Z

Currently we used to fall back and use non-streaming
version but we can instead just use the streaming
version instead.

Does not fix any apparent bug, but it just allows
for avoiding any checksum calculation for large buffers.

krisis · 2017-07-11T08:19:59Z

api.go

@@ -700,7 +700,8 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
 	case signerType.IsV2():
 		// Add signature version '2' authorization header.
 		req = s3signer.SignV2(*req, accessKeyID, secretAccessKey)
-	case signerType.IsStreamingV4() && method == "PUT":
+	case metadata.objectName != "" && method == "PUT":


This change has the following user-visible impact.

signature type set in configuration or overridden via overrideSignerType` is not honored, for all PUT requests on object resources. This may confuse existing users.

removes some performance advantages that were available to users previously.
E.g, user uploading an object of known size over HTTPS would like to benefit from not having to compute sha256 sum of the payload. It looks like this request will be signed using streaming signature forcing sha256 sum computation for every 'chunk'.

Correct we just default to sha256 anyways.. This is faster than actually doing md5sum with https in the benchmarks.. Defaulting to streaming was already done for all PUT operations, this change just goes one step further and enables that also for situation where size of the object is not known.

Streaming signature is a subset of V4 not a different auth mechanism in itself . That is why the differentiation is removed and implemented in such a way that .. if V4 is configured and object is being uploaded they all default to streaming signature implementation.

Correct we just default to sha256 anyways.. This is faster than actually doing md5sum with https in the benchmarks..

How does the performance of streaming signature compare with not computing sha256 sum of payload when object is being uploaded over a HTTPS connection using signature v4? Is streaming signature based upload (which computes sha256 sum of every chunk) faster than UNSIGNED PAYLOAD using signature v4 over HTTPS?

Even with unsigned payload we calculate content-md5 . That is what slows it down.. but you are right I can change that for secure connection can just skip this as well.

krisis

The code changes look good to me. I have a minor (yet important) comment w.r.t a comment.

krisis · 2017-07-14T08:32:41Z

api.go

@@ -700,7 +700,11 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
 	case signerType.IsV2():
 		// Add signature version '2' authorization header.
 		req = s3signer.SignV2(*req, accessKeyID, secretAccessKey)
-	case signerType.IsStreamingV4() && method == "PUT":
+	case metadata.objectName != "" && method == "PUT" && metadata.customHeader.Get("X-Amz-Copy-Source") == "" && !c.secure:
+		// Following condition is only invoked when objectName is set,


This comment describes the low-level details which are available in the code. We could describe the same at a higher-level as,
// Streaming signature is used by default for a PUT object request on a HTTP object storage server.
In a higher-level description the reader is required to make the connection between the goal and the case expression that represents it. This seems reasonable to me. Thoughts?

Yes.. done.

vadmeste · 2017-07-14T21:55:40Z

api.go

@@ -700,7 +700,8 @@ func (c Client) newRequest(method string, metadata requestMetadata) (req *http.R
 	case signerType.IsV2():
 		// Add signature version '2' authorization header.
 		req = s3signer.SignV2(*req, accessKeyID, secretAccessKey)
-	case signerType.IsStreamingV4() && method == "PUT":
+	case metadata.objectName != "" && method == "PUT" && metadata.customHeader.Get("X-Amz-Copy-Source") == "" && !c.secure:


@harshavardhana, can you add a comment to explain why we check c.secure ?

Because we don't want to do TLS sha256.

vadmeste · 2017-07-14T22:04:23Z

Looks like minio-go tests are not passing here:

--- FAIL: TestCorePutObject (5.90s)
        core_test.go:354: Error expected: nil, got:  <nil>

Could be not related to this PR.

harshavardhana · 2017-07-14T22:15:05Z

Looks like minio-go tests are not passing here:

--- FAIL: TestCorePutObject (5.90s)
core_test.go:354: Error expected: nil, got:
Could be not related to this PR.

This looks like some other problem. There shouldn't be an error here.

Currently we used to fall back and use non-streaming version but we can instead just use the streaming version instead. Does not fix any apparent bug, but it just allows for avoiding any checksum calculation for large buffers.

harshavardhana · 2017-07-15T21:51:07Z

@vadmeste can you take a look again the PR is updated.

vadmeste · 2017-07-16T14:17:16Z

api-put-object.go

 	if size < minPartSize {
 		return c.putObjectNoChecksum(bucketName, objectName, reader, size, metadata, progress)
 	}

 	// For all sizes greater than 64MiB do multipart.
 	return c.putObjectMultipartStream(bucketName, objectName, reader, size, metadata, progress)
 }
+
+func (c Client) putObjectMultipartStreamNoLength(bucketName, objectName string, reader io.Reader, size int64,


Sorry for the delay. I don't understand one thing: why are we passing size parameter here though this function name is "putObjectMultipartStreamNoLength" (nolength). Besides, this function seems to be called only when size < 0.

I guess we can change that.

Can this be done in a later PR which refactors PutObject and get this merged ?

Can this be done in a later PR which refactors PutObject and get this merged ?

Sure

vadmeste

LGTM

harshavardhana force-pushed the avoid-mutation branch from 37f8101 to 60a9531 Compare July 10, 2017 05:14

deekoder requested review from krisis and donatello July 10, 2017 19:06

krisis reviewed Jul 11, 2017

View reviewed changes

harshavardhana force-pushed the avoid-mutation branch 2 times, most recently from 5f97bac to fdfe15c Compare July 11, 2017 18:44

krisis requested changes Jul 14, 2017

View reviewed changes

harshavardhana force-pushed the avoid-mutation branch from fdfe15c to fd78ae2 Compare July 14, 2017 09:05

krisis approved these changes Jul 14, 2017

View reviewed changes

deekoder requested review from vadmeste and removed request for donatello July 14, 2017 21:43

vadmeste reviewed Jul 14, 2017

View reviewed changes

harshavardhana force-pushed the avoid-mutation branch from fd78ae2 to fa7151d Compare July 14, 2017 22:19

PutObject streams with unknown size use streaming V4.

0c1d594

Currently we used to fall back and use non-streaming version but we can instead just use the streaming version instead. Does not fix any apparent bug, but it just allows for avoiding any checksum calculation for large buffers.

harshavardhana force-pushed the avoid-mutation branch from fa7151d to 0c1d594 Compare July 14, 2017 22:26

vadmeste reviewed Jul 16, 2017

View reviewed changes

vadmeste approved these changes Jul 18, 2017

View reviewed changes

vadmeste merged commit 75c1ccd into minio:master Jul 18, 2017

harshavardhana deleted the avoid-mutation branch July 18, 2017 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PutObject streams with unknown size use streaming V4. #750

PutObject streams with unknown size use streaming V4. #750

harshavardhana commented Jul 10, 2017

krisis Jul 11, 2017

harshavardhana Jul 11, 2017

krisis Jul 11, 2017

harshavardhana Jul 11, 2017

krisis left a comment

krisis Jul 14, 2017 •

edited

harshavardhana Jul 14, 2017

vadmeste Jul 14, 2017 •

edited

harshavardhana Jul 14, 2017 •

edited

vadmeste commented Jul 14, 2017

harshavardhana commented Jul 14, 2017

harshavardhana commented Jul 15, 2017

vadmeste Jul 16, 2017 •

edited

harshavardhana Jul 16, 2017

harshavardhana Jul 18, 2017

vadmeste Jul 18, 2017

vadmeste left a comment

PutObject streams with unknown size use streaming V4. #750

PutObject streams with unknown size use streaming V4. #750

Conversation

harshavardhana commented Jul 10, 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

krisis left a comment

Choose a reason for hiding this comment

krisis Jul 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadmeste Jul 14, 2017 • edited

Choose a reason for hiding this comment

harshavardhana Jul 14, 2017 • edited

Choose a reason for hiding this comment

vadmeste commented Jul 14, 2017

harshavardhana commented Jul 14, 2017

harshavardhana commented Jul 15, 2017

vadmeste Jul 16, 2017 • edited

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

Choose a reason for hiding this comment

krisis Jul 14, 2017 •

edited

vadmeste Jul 14, 2017 •

edited

harshavardhana Jul 14, 2017 •

edited

vadmeste Jul 16, 2017 •

edited