cat: Ignore Stat() error and pass to Get() #2166

vadmeste · 2017-05-31T14:50:20Z

client-s3.Get() calls Stat() method because Get() is supposed to return
object metadata. This PR makes a change in client interface so metadata
is returned in Stat() method. This is beneficial for us to avoid calling
HEAD method in S3 when run mc cat

Fixes #2156

codecov-io · 2017-06-02T11:33:48Z

Codecov Report

Merging #2166 into master will decrease coverage by 0.04%.
The diff coverage is 9.67%.

@@            Coverage Diff            @@
##           master   #2166      +/-   ##
=========================================
- Coverage    8.91%   8.87%   -0.05%     
=========================================
  Files          92      92              
  Lines        6863    6865       +2     
=========================================
- Hits          612     609       -3     
- Misses       6119    6125       +6     
+ Partials      132     131       -1

Impacted Files	Coverage Δ
cmd/cat-main.go	`0% <0%> (ø)`	⬆️
cmd/pipe-main.go	`0% <0%> (ø)`	⬆️
cmd/common-methods.go	`0% <0%> (ø)`	⬆️
cmd/client-s3.go	`13.85% <18.18%> (-0.3%)`	⬇️
cmd/client-fs.go	`29.06% <50%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be33703...6b6b624. Read the comment docs.

vadmeste · 2017-06-02T14:43:37Z

Not WIP anymore. Ready for testing and reviewing and discussing design change of client interface.

harshavardhana

Overall the approach is fine, what needs to be done is a full work perhaps in the next PR optimizing all the areas to avoid double calls like these.

krisis · 2017-06-05T08:43:51Z

cmd/client-fs.go

@@ -451,7 +451,7 @@ func (f *fsClient) Copy(source string, size int64, progress io.Reader) *probe.Er

 // get - get wrapper returning reader and additional metadata if any.


The comment on additional metadata is superfluous with this change, please remove it.

The comment on additional metadata is superfluous with this change, please remove it.

Done

krisis · 2017-06-05T08:50:30Z

cmd/client-s3.go

 	}
 	objectMetadata.URL = *c.targetURL
 	objectMetadata.Time = objectStat.LastModified
 	objectMetadata.Size = objectStat.Size
 	objectMetadata.Type = os.FileMode(0664)
-	return objectMetadata, nil
+
+	metadata := objectStat.Metadata


If metadata can be constructed from *clientContent then why should we return it separately? Also, we ignore the returned metadatain most (other) places.

If metadata can be constructed from *clientContent

Not sure what do you mean, clientContent is defined in mc and metadata is returned from minio-go. so metadata is not constructed from clientContent.

Isn't it possible to construct a map[string]string (metadata) from objectMetadata even outside this function?

Are you suggesting that we should merge the returned metadata with objectMetadata instead of returning it separately ?

Yes. IIUC, we don't use metadata everywhere. So, I am suggesting we construct map[string] string wherever required with corresponding values found in objectMetadata from the Stat method. Does that make sense?

Yes. IIUC, we don't use metadata everywhere. So, I am suggesting we construct map[string] string wherever required with corresponding values found in objectMetadata from the Stat method. Does that make sense?

Yes, I agree.

So the client interface will be changed from:

40 type Client interface { 41 // Common operations 42 Stat(isIncomplete bool) (content *clientContent, metadata map[string][]string, err *probe.Error)

To

40 type Client interface { 41 // Common operations 42 Stat(isIncomplete bool) (content *clientContent, err *probe.Error)

and clientContent will have new Metadata additional field:

75 type clientContent struct { 76 URL clientURL 77 Time time.Time 78 Size int64 79 Type os.FileMode Metadata map[string][]string 80 Err *probe.Error 81 }

@abperiasamy, @harshavardhana what do you think ?

Yes clientContent can have the metadata which is completely fine @vadmeste and it is the right place IMHO.

Yes clientContent can have the metadata which is completely fine @vadmeste and it is the right place IMHO.

Okay, will do.

client-s3.Get() calls Stat() method because Get() is supposed to return object metadata. This PR makes a change in client interface so metadata is returned in Stat() method. This is beneficial for us to avoid calling HEAD method in S3 when run `mc cat`

In case of cache gateway, HEAD objects returns 404 even though objects are gettable. This PR ignores Stat() error since its purpose in case of cat is to check if the download object size is equal to what the server announced.

vadmeste · 2017-06-05T20:11:26Z

I made further changes, can you review again ? this PR was partly fixing the issue that I wanted to address. But now cat command is also changed.

@deekoder, can you block this PR? because it cann't be fully tested unless Minio cache gateway feature gets merged.

harshavardhana · 2017-06-06T21:35:59Z

ping @krisis anything else from your end on this PR?

krisis · 2017-06-07T04:17:37Z

cmd/cat-main.go

-		// Ignore size for filesystem objects since os.Stat() would not
-		// return proper size all the time, for example with /proc files.
-		if client.GetURL().Type == objectStorage {
+		if err == nil && client.GetURL().Type == objectStorage {


Previously, we return immediately if err != nil from url2Stat. Why are we ignoring the error in this change?

Previously, we return immediately if err != nil from url2Stat. Why are we ignoring the error in this change?

In the first place, url2Stat() is only needed to fetch the size of the remote object so we can check that we downloaded all the object data from the server. So actually, testing on url2Stat() is not crucial since we call getSourceStreamFromURL() later.

But why we ignore that error now? it is because we need to enhance support of mc with the new minio cache gateway. In some cases, with the cache gateway, a HEAD object returns 404 but GET can return 200 with object data. Refer to #2156 or ping me online.

@vadmeste, OK. I am withholding approval of PR pending test. Otherwise, the change looks good to me.

krisis · 2017-06-23T08:30:14Z

cmd/client-s3.go

@@ -894,12 +884,14 @@ func (c *s3Client) Stat(isIncomplete bool) (*clientContent, *probe.Error) {
 			objectMetadata.Time = objectStat.LastModified
 			objectMetadata.Size = objectStat.Size
 			objectMetadata.Type = os.FileMode(0664)
+			objectMetadata.Metadata = map[string][]string{}


why should we try to find the given object by listing the objects in the bucket for fetching stat information? We can directly call c.api.StatObject(bucket, object), right?

@krisis, Stat() in client-s3 is supposed to be able to Stat() a directory but StatObject() of minio-go cannot.

@vadmeste Ah, didn't see that. Thanks for the explanation.

vadmeste · 2017-06-29T13:34:30Z

Can we allow this to be merged? reviewers can mark this PR reviewed and not tested

krisis

Not tested, but changes LGTM.

deekoder · 2017-06-29T16:14:29Z

I am not sure if I should merge it if its not tested.

vadmeste · 2017-06-29T16:20:42Z

I am not sure if I should merge it if its not tested.

This PR was open for a long time, we can't test because Minio cache is not ready yet, also by time we will have to rebase it many times.

Also I'll have less opened issues in my buzz list if we merge this :)

krisis · 2017-06-29T16:26:44Z

@deekoder Tested locally (using mc cat --debug) and works as expected. Like @vadmeste mentioned, this change can't be tested in the context of edge cache.

harshavardhana · 2017-06-29T18:07:25Z

@deekoder Tested locally (using mc cat --debug) and works as expected. Like @vadmeste mentioned, this change can't be tested in the context of edge cache.

IMO its a good change to have anycase.

deekoder · 2017-06-30T23:34:30Z

Since its approved by the reviewers, can i merge it? Its not clear whether testability was an issue?

vadmeste · 2017-06-30T23:37:52Z

Since its approved by the reviewers, can i merge it?

Yes you can.

Its not clear whether testability was an issue?

Because Minio cache is not ready yet.. reviewers checked debug log to confirm it is working as expected

vadmeste changed the title ~~client: Avoid returning metadata with Get()~~ [WIP] client: Avoid returning metadata with Get() May 31, 2017

vadmeste force-pushed the issues/2156 branch from 5b0c361 to 4f0a324 Compare June 2, 2017 11:31

vadmeste force-pushed the issues/2156 branch from 4f0a324 to 219d800 Compare June 2, 2017 14:32

vadmeste changed the title ~~[WIP] client: Avoid returning metadata with Get()~~ client: Avoid returning metadata with Get() Jun 2, 2017

deekoder requested review from harshavardhana and krisis June 2, 2017 18:26

harshavardhana approved these changes Jun 2, 2017

View reviewed changes

krisis requested changes Jun 5, 2017

View reviewed changes

vadmeste force-pushed the issues/2156 branch from 219d800 to 7143149 Compare June 5, 2017 12:00

vadmeste added 2 commits June 5, 2017 20:21

cat: Ignore Stat() returned error

6b6b624

In case of cache gateway, HEAD objects returns 404 even though objects are gettable. This PR ignores Stat() error since its purpose in case of cat is to check if the download object size is equal to what the server announced.

vadmeste force-pushed the issues/2156 branch from 7143149 to 6b6b624 Compare June 5, 2017 19:48

vadmeste changed the title ~~client: Avoid returning metadata with Get()~~ cat: Ignore Stat() error and pass to Get() Jun 5, 2017

harshavardhana approved these changes Jun 5, 2017

View reviewed changes

krisis reviewed Jun 7, 2017

View reviewed changes

krisis reviewed Jun 23, 2017

View reviewed changes

krisis approved these changes Jun 29, 2017

View reviewed changes

deekoder merged commit 1b9bead into minio:master Jul 4, 2017

vadmeste deleted the issues/2156 branch May 29, 2018 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cat: Ignore Stat() error and pass to Get() #2166

cat: Ignore Stat() error and pass to Get() #2166

vadmeste commented May 31, 2017 •

edited

codecov-io commented Jun 2, 2017 •

edited

vadmeste commented Jun 2, 2017

harshavardhana left a comment

krisis Jun 5, 2017

vadmeste Jun 5, 2017

krisis Jun 5, 2017 •

edited

vadmeste Jun 5, 2017

krisis Jun 5, 2017

vadmeste Jun 5, 2017 •

edited

krisis Jun 5, 2017

vadmeste Jun 5, 2017

harshavardhana Jun 5, 2017

vadmeste Jun 5, 2017

vadmeste commented Jun 5, 2017

harshavardhana commented Jun 6, 2017

krisis Jun 7, 2017

vadmeste Jun 7, 2017

krisis Jun 7, 2017

krisis Jun 23, 2017

vadmeste Jun 23, 2017

krisis Jun 29, 2017

vadmeste commented Jun 29, 2017

krisis left a comment

deekoder commented Jun 29, 2017

vadmeste commented Jun 29, 2017

krisis commented Jun 29, 2017

harshavardhana commented Jun 29, 2017

deekoder commented Jun 30, 2017

vadmeste commented Jun 30, 2017

		@@ -451,7 +451,7 @@ func (f fsClient) Copy(source string, size int64, progress io.Reader) probe.Er

		// get - get wrapper returning reader and additional metadata if any.

cat: Ignore Stat() error and pass to Get() #2166

cat: Ignore Stat() error and pass to Get() #2166

Conversation

vadmeste commented May 31, 2017 • edited

codecov-io commented Jun 2, 2017 • edited

Codecov Report

vadmeste commented Jun 2, 2017

harshavardhana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisis Jun 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadmeste Jun 5, 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

Choose a reason for hiding this comment

vadmeste commented Jun 5, 2017

harshavardhana commented Jun 6, 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

vadmeste commented Jun 29, 2017

krisis left a comment

Choose a reason for hiding this comment

deekoder commented Jun 29, 2017

vadmeste commented Jun 29, 2017

krisis commented Jun 29, 2017

harshavardhana commented Jun 29, 2017

deekoder commented Jun 30, 2017

vadmeste commented Jun 30, 2017

vadmeste commented May 31, 2017 •

edited

codecov-io commented Jun 2, 2017 •

edited

krisis Jun 5, 2017 •

edited

vadmeste Jun 5, 2017 •

edited