Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Jul 4, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bedaee2
client: Avoid returning metadata with Get()
vadmeste May 31, 2017
6b6b624
cat: Ignore Stat() returned error
vadmeste Jun 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions cmd/cat-main.go
View file
Open in desktop
Expand Up @@ -83,16 +83,16 @@ func catURL(sourceURL string) *probe.Error {
reader = os.Stdin
default:
var err *probe.Error
// Try to stat the object, the purpose is to extract the
// size of S3 object so we can check if the size of the
// downloaded object is equal to the original one. FS files
// are ignored since some of them have zero size though they
// have contents like files under /proc.
client, content, err := url2Stat(sourceURL)
if err != nil {
return err.Trace(sourceURL)
}
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

size = content.Size
}
if reader, err = getSourceStream(sourceURL); err != nil {
if reader, err = getSourceStreamFromURL(sourceURL); err != nil {
return err.Trace(sourceURL)
}
}
Expand Down
19 changes: 9 additions & 10 deletions cmd/client-fs.go
View file
Open in desktop
Expand Up @@ -449,9 +449,8 @@ func (f *fsClient) Copy(source string, size int64, progress io.Reader) *probe.Er
return nil
}

// get - get wrapper returning reader and additional metadata if any.
// currently only returns metadata.
func (f *fsClient) get() (io.Reader, map[string][]string, *probe.Error) {
// get - get wrapper returning object reader.
func (f *fsClient) get() (io.Reader, *probe.Error) {
tmppath := f.PathURL.Path
// Golang strips trailing / if you clean(..) or
// EvalSymlinks(..). Adding '.' prevents it from doing so.
Expand All @@ -463,21 +462,18 @@ func (f *fsClient) get() (io.Reader, map[string][]string, *probe.Error) {
_, e := filepath.EvalSymlinks(tmppath)
if e != nil {
err := f.toClientError(e, f.PathURL.Path)
return nil, nil, err.Trace(f.PathURL.Path)
return nil, err.Trace(f.PathURL.Path)
}
fileData, e := os.Open(f.PathURL.Path)
if e != nil {
err := f.toClientError(e, f.PathURL.Path)
return nil, nil, err.Trace(f.PathURL.Path)
return nil, err.Trace(f.PathURL.Path)
}
metadata := map[string][]string{
"Content-Type": {guessURLContentType(f.PathURL.Path)},
}
return fileData, metadata, nil
return fileData, nil
}

// Get returns reader and any additional metadata.
func (f *fsClient) Get() (io.Reader, map[string][]string, *probe.Error) {
func (f *fsClient) Get() (io.Reader, *probe.Error) {
return f.get()
}

Expand Down Expand Up @@ -1021,6 +1017,9 @@ func (f *fsClient) Stat(isIncomplete bool) (content *clientContent, err *probe.E
content.Size = st.Size()
content.Time = st.ModTime()
content.Type = st.Mode()
content.Metadata = map[string][]string{
"Content-Type": {guessURLContentType(f.PathURL.Path)},
}
return content, nil
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/client-fs_test.go
View file
Open in desktop
Expand Up @@ -274,7 +274,7 @@ func (s *TestSuite) TestGet(c *C) {
c.Assert(err, IsNil)
c.Assert(n, Equals, int64(len(data)))

reader, _, err = fsClient.Get()
reader, err = fsClient.Get()
c.Assert(err, IsNil)
var results bytes.Buffer
_, e = io.Copy(&results, reader)
Expand Down Expand Up @@ -302,7 +302,7 @@ func (s *TestSuite) TestGetRange(c *C) {
c.Assert(err, IsNil)
c.Assert(n, Equals, int64(len(data)))

reader, _, err = fsClient.Get()
reader, err = fsClient.Get()
c.Assert(err, IsNil)
var results bytes.Buffer
buf := make([]byte, 5)
Expand Down
35 changes: 16 additions & 19 deletions cmd/client-s3.go
View file
Open in desktop
Expand Up @@ -497,40 +497,27 @@ func (c *s3Client) Watch(params watchParams) (*watchObject, *probe.Error) {
}

// Get - get object with metadata.
func (c *s3Client) Get() (io.Reader, map[string][]string, *probe.Error) {
func (c *s3Client) Get() (io.Reader, *probe.Error) {
bucket, object := c.url2BucketAndObject()
reader, e := c.api.GetObject(bucket, object)
if e != nil {
errResponse := minio.ToErrorResponse(e)
if errResponse.Code == "NoSuchBucket" {
return nil, nil, probe.NewError(BucketDoesNotExist{
return nil, probe.NewError(BucketDoesNotExist{
Bucket: bucket,
})
}
if errResponse.Code == "InvalidBucketName" {
return nil, nil, probe.NewError(BucketInvalid{
return nil, probe.NewError(BucketInvalid{
Bucket: bucket,
})
}
if errResponse.Code == "NoSuchKey" || errResponse.Code == "InvalidArgument" {
return nil, nil, probe.NewError(ObjectMissing{})
}
return nil, nil, probe.NewError(e)
}
objInfo, e := reader.Stat()
if e != nil {
errResponse := minio.ToErrorResponse(e)
if errResponse.Code == "AccessDenied" {
return nil, nil, probe.NewError(PathInsufficientPermission{Path: c.targetURL.String()})
}
if errResponse.Code == "NoSuchKey" || errResponse.Code == "InvalidArgument" {
return nil, nil, probe.NewError(ObjectMissing{})
return nil, probe.NewError(ObjectMissing{})
}
return nil, nil, probe.NewError(e)
return nil, probe.NewError(e)
}
metadata := objInfo.Metadata
metadata.Set("Content-Type", objInfo.ContentType)
return reader, metadata, nil
return reader, nil
}

// Copy - copy object
Expand Down Expand Up @@ -849,6 +836,7 @@ func (c *s3Client) Stat(isIncomplete bool) (*clientContent, *probe.Error) {
bucketMetadata := &clientContent{}
bucketMetadata.URL = *c.targetURL
bucketMetadata.Type = os.ModeDir
bucketMetadata.Metadata = map[string][]string{}

return bucketMetadata, nil
}
Expand All @@ -872,12 +860,14 @@ func (c *s3Client) Stat(isIncomplete bool) (*clientContent, *probe.Error) {
objectMetadata.Time = objectMultipartInfo.Initiated
objectMetadata.Size = objectMultipartInfo.Size
objectMetadata.Type = os.FileMode(0664)
objectMetadata.Metadata = map[string][]string{}
return objectMetadata, nil
}

if strings.HasSuffix(objectMultipartInfo.Key, string(c.targetURL.Separator)) {
objectMetadata.URL = *c.targetURL
objectMetadata.Type = os.ModeDir
objectMetadata.Metadata = map[string][]string{}
return objectMetadata, nil
}
}
Expand All @@ -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{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return objectMetadata, nil
}

if strings.HasSuffix(objectStat.Key, string(c.targetURL.Separator)) {
objectMetadata.URL = *c.targetURL
objectMetadata.Type = os.ModeDir
objectMetadata.Metadata = map[string][]string{}
return objectMetadata, nil
}
}
Expand Down Expand Up @@ -928,6 +920,11 @@ func (c *s3Client) Stat(isIncomplete bool) (*clientContent, *probe.Error) {
objectMetadata.Time = objectStat.LastModified
objectMetadata.Size = objectStat.Size
objectMetadata.Type = os.FileMode(0664)

metadata := objectStat.Metadata
metadata.Set("Content-Type", objectStat.ContentType)
objectMetadata.Metadata = metadata

return objectMetadata, nil
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/client-s3_test.go
View file
Open in desktop
Expand Up @@ -229,7 +229,7 @@ func (s *TestSuite) TestObjectOperations(c *C) {
c.Assert(err, IsNil)
c.Assert(n, Equals, int64(len(object.data)))

reader, _, err = s3c.Get()
reader, err = s3c.Get()
c.Assert(err, IsNil)
var buffer bytes.Buffer
{
Expand Down
13 changes: 7 additions & 6 deletions cmd/client.go
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@ type Client interface {
Copy(source string, size int64, progress io.Reader) *probe.Error

// I/O operations with metadata.
Get() (reader io.Reader, metadata map[string][]string, err *probe.Error)
Get() (reader io.Reader, err *probe.Error)
Put(reader io.Reader, size int64, metadata map[string][]string, progress io.Reader) (n int64, err *probe.Error)

// I/O operations with expiration
Expand All @@ -73,11 +73,12 @@ type Client interface {

// Content container for content metadata
type clientContent struct {
URL clientURL
Time time.Time
Size int64
Type os.FileMode
Err *probe.Error
URL clientURL
Time time.Time
Size int64
Type os.FileMode
Metadata map[string][]string
Err *probe.Error
}

// Config - see http://docs.amazonwebservices.com/AmazonS3/latest/dev/index.html?RESTAuthentication.html
Expand Down
34 changes: 21 additions & 13 deletions cmd/common-methods.go
View file
Open in desktop
Expand Up @@ -64,31 +64,39 @@ func isAliasURLDir(aliasURL string) bool {
return strings.HasSuffix(pathURL, "/")
}

// getSource gets a reader from URL.
func getSourceStream(urlStr string) (reader io.Reader, err *probe.Error) {
// getSourceStreamFromURL gets a reader from URL.
func getSourceStreamFromURL(urlStr string) (reader io.Reader, err *probe.Error) {
alias, urlStrFull, _, err := expandAlias(urlStr)
if err != nil {
return nil, err.Trace(urlStr)
}
reader, _, err = getSourceStreamFromAlias(alias, urlStrFull)
reader, _, err = getSourceStream(alias, urlStrFull, false)
return reader, err
}

// getSourceStreamFromAlias gets a reader from URL.
func getSourceStreamFromAlias(alias string, urlStr string) (reader io.Reader, metadata map[string][]string, err *probe.Error) {
// getSourceStream gets a reader from URL.
func getSourceStream(alias string, urlStr string, fetchStat bool) (reader io.Reader, metadata map[string][]string, err *probe.Error) {
sourceClnt, err := newClientFromAlias(alias, urlStr)
if err != nil {
return nil, nil, err.Trace(alias, urlStr)
}
reader, metadata, err = sourceClnt.Get()
reader, err = sourceClnt.Get()
if err != nil {
return nil, nil, err.Trace(alias, urlStr)
}
metadata = map[string][]string{}
if fetchStat {
st, err := sourceClnt.Stat(false)
if err != nil {
return nil, nil, err.Trace(alias, urlStr)
}
metadata = st.Metadata
}
return reader, metadata, nil
}

// putTargetStreamFromAlias writes to URL from Reader.
func putTargetStreamFromAlias(alias string, urlStr string, reader io.Reader, size int64, metadata map[string][]string, progress io.Reader) (int64, *probe.Error) {
// putTargetStream writes to URL from Reader.
func putTargetStream(alias string, urlStr string, reader io.Reader, size int64, metadata map[string][]string, progress io.Reader) (int64, *probe.Error) {
targetClnt, err := newClientFromAlias(alias, urlStr)
if err != nil {
return 0, err.Trace(alias, urlStr)
Expand All @@ -100,8 +108,8 @@ func putTargetStreamFromAlias(alias string, urlStr string, reader io.Reader, siz
return n, nil
}

// putTargetStream writes to URL from reader. If length=-1, read until EOF.
func putTargetStream(urlStr string, reader io.Reader, size int64) (int64, *probe.Error) {
// putTargetStreamWithURL writes to URL from reader. If length=-1, read until EOF.
func putTargetStreamWithURL(urlStr string, reader io.Reader, size int64) (int64, *probe.Error) {
alias, urlStrFull, _, err := expandAlias(urlStr)
if err != nil {
return 0, err.Trace(alias, urlStr)
Expand All @@ -110,7 +118,7 @@ func putTargetStream(urlStr string, reader io.Reader, size int64) (int64, *probe
metadata := map[string][]string{
"Content-Type": {contentType},
}
return putTargetStreamFromAlias(alias, urlStrFull, reader, size, metadata, nil)
return putTargetStream(alias, urlStrFull, reader, size, metadata, nil)
}

// copySourceToTargetURL copies to targetURL from source.
Expand Down Expand Up @@ -145,11 +153,11 @@ func uploadSourceToTargetURL(urls URLs, progress io.Reader) URLs {
}
} else {
// Proceed with regular stream copy.
reader, metadata, err := getSourceStreamFromAlias(sourceAlias, sourceURL.String())
reader, metadata, err := getSourceStream(sourceAlias, sourceURL.String(), true)
if err != nil {
return urls.WithError(err.Trace(sourceURL.String()))
}
_, err = putTargetStreamFromAlias(targetAlias, targetURL.String(), reader, length, metadata, progress)
_, err = putTargetStream(targetAlias, targetURL.String(), reader, length, metadata, progress)
if err != nil {
return urls.WithError(err.Trace(targetURL.String()))
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/pipe-main.go
View file
Open in desktop
Expand Up @@ -68,7 +68,7 @@ func pipe(targetURL string) *probe.Error {
// Stream from stdin to multiple objects until EOF.
// Ignore size, since os.Stat() would not return proper size all the time
// for local filesystem for example /proc files.
_, err := putTargetStream(targetURL, os.Stdin, -1)
_, err := putTargetStreamWithURL(targetURL, os.Stdin, -1)
// TODO: See if this check is necessary.
switch e := err.ToGoError().(type) {
case *os.PathError:
Expand Down