From 64998fc4abe4dcff550f7b8739d83dad2fecdf6c Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 1 May 2019 22:06:57 -0700 Subject: [PATCH] Remove delayIsLeaf requirement simplify ListObjects further (#7593) --- cmd/disk-cache.go | 29 +++--- cmd/fs-v1.go | 24 +---- cmd/gateway-common.go | 4 +- cmd/gateway/hdfs/gateway-hdfs.go | 40 +------- cmd/object-api-common.go | 4 +- cmd/object-api-listobjects_test.go | 111 ++++++++++---------- cmd/tree-walk.go | 100 ++---------------- cmd/tree-walk_test.go | 157 +++++------------------------ cmd/xl-sets.go | 30 ++---- cmd/xl-v1-common.go | 31 ------ cmd/xl-v1-list-objects.go | 14 +-- 11 files changed, 130 insertions(+), 414 deletions(-) diff --git a/cmd/disk-cache.go b/cmd/disk-cache.go index b8d92a8eb..f450be307 100644 --- a/cmd/disk-cache.go +++ b/cmd/disk-cache.go @@ -375,7 +375,7 @@ func (c cacheObjects) GetObjectInfo(ctx context.Context, bucket, object string, // Returns function "listDir" of the type listDirFunc. // isLeaf - is used by listDir function to check if an entry is a leaf or non-leaf entry. // disks - list of fsObjects -func listDirCacheFactory(isLeaf IsLeafFunc, disks []*cacheFSObjects) ListDirFunc { +func listDirCacheFactory(isLeaf func(string, string) bool, disks []*cacheFSObjects) ListDirFunc { listCacheDirs := func(bucket, prefixDir, prefixEntry string) (dirs []string) { var entries []string for _, disk := range disks { @@ -391,6 +391,12 @@ func listDirCacheFactory(isLeaf IsLeafFunc, disks []*cacheFSObjects) ListDirFunc continue } + for i := range entries { + if isLeaf(bucket, entries[i]) { + entries[i] = strings.TrimSuffix(entries[i], slashSeparator) + } + } + // Filter entries that have the prefix prefixEntry. entries = filterMatchingPrefix(entries, prefixEntry) dirs = append(dirs, entries...) @@ -399,7 +405,7 @@ func listDirCacheFactory(isLeaf IsLeafFunc, disks []*cacheFSObjects) ListDirFunc } // listDir - lists all the entries at a given prefix and given entry in the prefix. - listDir := func(bucket, prefixDir, prefixEntry string) (mergedEntries []string, delayIsLeaf bool) { + listDir := func(bucket, prefixDir, prefixEntry string) (mergedEntries []string) { cacheEntries := listCacheDirs(bucket, prefixDir, prefixEntry) for _, entry := range cacheEntries { // Find elements in entries which are not in mergedEntries @@ -411,7 +417,7 @@ func listDirCacheFactory(isLeaf IsLeafFunc, disks []*cacheFSObjects) ListDirFunc mergedEntries = append(mergedEntries, entry) sort.Strings(mergedEntries) } - return mergedEntries, false + return mergedEntries } return listDir } @@ -430,25 +436,16 @@ func (c cacheObjects) listCacheObjects(ctx context.Context, bucket, prefix, mark walkResultCh, endWalkCh := c.listPool.Release(listParams{bucket, recursive, marker, prefix}) if walkResultCh == nil { endWalkCh = make(chan struct{}) - isLeaf := func(bucket, object string) bool { + + listDir := listDirCacheFactory(func(bucket, object string) bool { fs, err := c.cache.getCacheFS(ctx, bucket, object) if err != nil { return false } _, err = fs.getObjectInfo(ctx, bucket, object) return err == nil - } - - isLeafDir := func(bucket, object string) bool { - fs, err := c.cache.getCacheFS(ctx, bucket, object) - if err != nil { - return false - } - return fs.isObjectDir(bucket, object) - } - - listDir := listDirCacheFactory(isLeaf, c.cache.cfs) - walkResultCh = startTreeWalk(ctx, bucket, prefix, marker, recursive, listDir, isLeaf, isLeafDir, endWalkCh) + }, c.cache.cfs) + walkResultCh = startTreeWalk(ctx, bucket, prefix, marker, recursive, listDir, endWalkCh) } for i := 0; i < maxKeys; { diff --git a/cmd/fs-v1.go b/cmd/fs-v1.go index b6e6e5fba..43fa39477 100644 --- a/cmd/fs-v1.go +++ b/cmd/fs-v1.go @@ -1001,9 +1001,9 @@ func (fs *FSObjects) DeleteObject(ctx context.Context, bucket, object string) er // Returns function "listDir" of the type listDirFunc. // isLeaf - is used by listDir function to check if an entry // is a leaf or non-leaf entry. -func (fs *FSObjects) listDirFactory(isLeaf IsLeafFunc) ListDirFunc { +func (fs *FSObjects) listDirFactory() ListDirFunc { // listDir - lists all the entries at a given prefix and given entry in the prefix. - listDir := func(bucket, prefixDir, prefixEntry string) (entries []string, delayIsLeaf bool) { + listDir := func(bucket, prefixDir, prefixEntry string) (entries []string) { var err error entries, err = readDir(pathJoin(fs.fsPath, bucket, prefixDir)) if err != nil && err != errFileNotFound { @@ -1011,7 +1011,7 @@ func (fs *FSObjects) listDirFactory(isLeaf IsLeafFunc) ListDirFunc { return } sort.Strings(entries) - return filterListEntries(bucket, prefixDir, entries, prefixEntry, isLeaf) + return filterMatchingPrefix(entries, prefixEntry) } // Return list factory instance. @@ -1097,22 +1097,8 @@ func (fs *FSObjects) getObjectETag(ctx context.Context, bucket, entry string, lo // ListObjects - list all objects at prefix upto maxKeys., optionally delimited by '/'. Maintains the list pool // state for future re-entrant list requests. func (fs *FSObjects) ListObjects(ctx context.Context, bucket, prefix, marker, delimiter string, maxKeys int) (loi ListObjectsInfo, e error) { - isLeaf := func(bucket, object string) bool { - // bucket argument is unused as we don't need to StatFile - // to figure if it's a file, just need to check that the - // object string does not end with "/". - return !hasSuffix(object, slashSeparator) - } - // Return true if the specified object is an empty directory - isLeafDir := func(bucket, object string) bool { - if !hasSuffix(object, slashSeparator) { - return false - } - return fs.isObjectDir(bucket, object) - } - listDir := fs.listDirFactory(isLeaf) - - return listObjects(ctx, fs, bucket, prefix, marker, delimiter, maxKeys, fs.listPool, isLeaf, isLeafDir, listDir, fs.getObjectInfo, fs.getObjectInfo) + return listObjects(ctx, fs, bucket, prefix, marker, delimiter, maxKeys, fs.listPool, + fs.listDirFactory(), fs.getObjectInfo, fs.getObjectInfo) } // ReloadFormat - no-op for fs, Valid only for XL. diff --git a/cmd/gateway-common.go b/cmd/gateway-common.go index 88529a8bd..008fc1aa9 100644 --- a/cmd/gateway-common.go +++ b/cmd/gateway-common.go @@ -44,8 +44,8 @@ var ( // ListObjects function alias. ListObjects = listObjects - // FilterListEntries function alias. - FilterListEntries = filterListEntries + // FilterMatchingPrefix function alias. + FilterMatchingPrefix = filterMatchingPrefix // IsStringEqual is string equal. IsStringEqual = isStringEqual diff --git a/cmd/gateway/hdfs/gateway-hdfs.go b/cmd/gateway/hdfs/gateway-hdfs.go index ea1ef9c62..b71b95ead 100644 --- a/cmd/gateway/hdfs/gateway-hdfs.go +++ b/cmd/gateway/hdfs/gateway-hdfs.go @@ -287,24 +287,9 @@ func (n *hdfsObjects) ListBuckets(ctx context.Context) (buckets []minio.BucketIn return buckets, nil } -func (n *hdfsObjects) isObjectDir(bucket, object string) bool { - f, err := n.clnt.Open(minio.PathJoin(hdfsSeparator, bucket, object)) - if err != nil { - return false - } - defer f.Close() - - entries, err := f.Readdir(1) - if err != nil { - return false - } - - return len(entries) == 0 -} - -func (n *hdfsObjects) listDirFactory(isLeaf minio.IsLeafFunc) minio.ListDirFunc { +func (n *hdfsObjects) listDirFactory() minio.ListDirFunc { // listDir - lists all the entries at a given prefix and given entry in the prefix. - listDir := func(bucket, prefixDir, prefixEntry string) (entries []string, delayIsLeaf bool) { + listDir := func(bucket, prefixDir, prefixEntry string) (entries []string) { f, err := n.clnt.Open(minio.PathJoin(hdfsSeparator, bucket, prefixDir)) if err != nil { if os.IsNotExist(err) { @@ -327,8 +312,7 @@ func (n *hdfsObjects) listDirFactory(isLeaf minio.IsLeafFunc) minio.ListDirFunc } } fis = nil - entries, delayIsLeaf = minio.FilterListEntries(bucket, prefixDir, entries, prefixEntry, isLeaf) - return entries, delayIsLeaf + return minio.FilterMatchingPrefix(entries, prefixEntry) } // Return list factory instance. @@ -337,27 +321,11 @@ func (n *hdfsObjects) listDirFactory(isLeaf minio.IsLeafFunc) minio.ListDirFunc // ListObjects lists all blobs in HDFS bucket filtered by prefix. func (n *hdfsObjects) ListObjects(ctx context.Context, bucket, prefix, marker, delimiter string, maxKeys int) (loi minio.ListObjectsInfo, err error) { - isLeaf := func(bucket, object string) bool { - // bucket argument is unused as we don't need to StatFile - // to figure if it's a file, just need to check that the - // object string does not end with "/". - return !strings.HasSuffix(object, hdfsSeparator) - } - - // Return true if the specified object is an empty directory - isLeafDir := func(bucket, object string) bool { - if !strings.HasSuffix(object, hdfsSeparator) { - return false - } - return n.isObjectDir(bucket, object) - } - listDir := n.listDirFactory(isLeaf) - getObjectInfo := func(ctx context.Context, bucket, entry string) (minio.ObjectInfo, error) { return n.GetObjectInfo(ctx, bucket, entry, minio.ObjectOptions{}) } - return minio.ListObjects(ctx, n, bucket, prefix, marker, delimiter, maxKeys, n.listPool, isLeaf, isLeafDir, listDir, getObjectInfo, getObjectInfo) + return minio.ListObjects(ctx, n, bucket, prefix, marker, delimiter, maxKeys, n.listPool, n.listDirFactory(), getObjectInfo, getObjectInfo) } // Check if the given error corresponds to ENOTEMPTY for unix diff --git a/cmd/object-api-common.go b/cmd/object-api-common.go index 71f779b81..ec3be7361 100644 --- a/cmd/object-api-common.go +++ b/cmd/object-api-common.go @@ -162,7 +162,7 @@ func removeListenerConfig(ctx context.Context, objAPI ObjectLayer, bucket string return objAPI.DeleteObject(ctx, minioMetaBucket, lcPath) } -func listObjects(ctx context.Context, obj ObjectLayer, bucket, prefix, marker, delimiter string, maxKeys int, tpool *TreeWalkPool, isLeaf IsLeafFunc, isLeafDir IsLeafDirFunc, listDir ListDirFunc, getObjInfo func(context.Context, string, string) (ObjectInfo, error), getObjectInfoDirs ...func(context.Context, string, string) (ObjectInfo, error)) (loi ListObjectsInfo, err error) { +func listObjects(ctx context.Context, obj ObjectLayer, bucket, prefix, marker, delimiter string, maxKeys int, tpool *TreeWalkPool, listDir ListDirFunc, getObjInfo func(context.Context, string, string) (ObjectInfo, error), getObjectInfoDirs ...func(context.Context, string, string) (ObjectInfo, error)) (loi ListObjectsInfo, err error) { if err := checkListObjsArgs(ctx, bucket, prefix, marker, delimiter, obj); err != nil { return loi, err } @@ -203,7 +203,7 @@ func listObjects(ctx context.Context, obj ObjectLayer, bucket, prefix, marker, d walkResultCh, endWalkCh := tpool.Release(listParams{bucket, recursive, marker, prefix}) if walkResultCh == nil { endWalkCh = make(chan struct{}) - walkResultCh = startTreeWalk(ctx, bucket, prefix, marker, recursive, listDir, isLeaf, isLeafDir, endWalkCh) + walkResultCh = startTreeWalk(ctx, bucket, prefix, marker, recursive, listDir, endWalkCh) } var objInfos []ObjectInfo diff --git a/cmd/object-api-listobjects_test.go b/cmd/object-api-listobjects_test.go index 22458c0bc..74b397c6e 100644 --- a/cmd/object-api-listobjects_test.go +++ b/cmd/object-api-listobjects_test.go @@ -35,8 +35,8 @@ func TestListObjects(t *testing.T) { } // Unit test for ListObjects in general. -func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { - +func testListObjects(obj ObjectLayer, instanceType string, t1 TestErrHandler) { + t, _ := t1.(*testing.T) testBuckets := []string{ // This bucket is used for testing ListObject operations. "test-bucket-list-object", @@ -66,7 +66,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {"obj0", "obj0", nil}, {"obj1", "obj1", nil}, {"obj2", "obj2", nil}, - {"z-empty-dir/", "", nil}, } for _, object := range testObjects { md5Bytes := md5.Sum([]byte(object.content)) @@ -96,7 +95,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-1. @@ -193,7 +191,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-10. @@ -205,7 +202,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-11. @@ -215,7 +211,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { Objects: []ObjectInfo{ {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-12. @@ -224,7 +219,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { IsTruncated: false, Objects: []ObjectInfo{ {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-13. @@ -238,7 +232,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-14. @@ -255,7 +248,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-15. @@ -270,7 +262,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-16. @@ -284,7 +275,6 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { {Name: "obj0"}, {Name: "obj1"}, {Name: "obj2"}, - {Name: "z-empty-dir/"}, }, }, // ListObjectsResult-17. @@ -535,61 +525,66 @@ func testListObjects(obj ObjectLayer, instanceType string, t TestErrHandler) { } for i, testCase := range testCases { - result, err := obj.ListObjects(context.Background(), testCase.bucketName, testCase.prefix, testCase.marker, testCase.delimeter, int(testCase.maxKeys)) - if err != nil && testCase.shouldPass { - t.Errorf("Test %d: %s: Expected to pass, but failed with: %s", i+1, instanceType, err.Error()) - } - if err == nil && !testCase.shouldPass { - t.Errorf("Test %d: %s: Expected to fail with \"%s\", but passed instead", i+1, instanceType, testCase.err.Error()) - } - // Failed as expected, but does it fail for the expected reason. - if err != nil && !testCase.shouldPass { - if !strings.Contains(err.Error(), testCase.err.Error()) { - t.Errorf("Test %d: %s: Expected to fail with error \"%s\", but instead failed with error \"%s\" instead", i+1, instanceType, testCase.err.Error(), err.Error()) + testCase := testCase + t.Run(fmt.Sprintf("Test%d-%s", i+1, instanceType), func(t *testing.T) { + result, err := obj.ListObjects(context.Background(), testCase.bucketName, + testCase.prefix, testCase.marker, testCase.delimeter, int(testCase.maxKeys)) + if err != nil && testCase.shouldPass { + t.Errorf("Test %d: %s: Expected to pass, but failed with: %s", i+1, instanceType, err.Error()) } - } - // Since there are cases for which ListObjects fails, this is - // necessary. Test passes as expected, but the output values - // are verified for correctness here. - if err == nil && testCase.shouldPass { - // The length of the expected ListObjectsResult.Objects - // should match in both expected result from test cases - // and in the output. On failure calling t.Fatalf, - // otherwise it may lead to index out of range error in - // assertion following this. - if len(testCase.result.Objects) != len(result.Objects) { - t.Fatalf("Test %d: %s: Expected number of object in the result to be '%d', but found '%d' objects instead", i+1, instanceType, len(testCase.result.Objects), len(result.Objects)) + if err == nil && !testCase.shouldPass { + t.Errorf("Test %d: %s: Expected to fail with \"%s\", but passed instead", i+1, instanceType, testCase.err.Error()) } - for j := 0; j < len(testCase.result.Objects); j++ { - if testCase.result.Objects[j].Name != result.Objects[j].Name { - t.Errorf("Test %d: %s: Expected object name to be \"%s\", but found \"%s\" instead", i+1, instanceType, testCase.result.Objects[j].Name, result.Objects[j].Name) + // Failed as expected, but does it fail for the expected reason. + if err != nil && !testCase.shouldPass { + if !strings.Contains(err.Error(), testCase.err.Error()) { + t.Errorf("Test %d: %s: Expected to fail with error \"%s\", but instead failed with error \"%s\" instead", i+1, instanceType, testCase.err.Error(), err.Error()) } - //FIXME: we should always check for ETag - if result.Objects[j].ETag == "" && !strings.HasSuffix(result.Objects[j].Name, slashSeparator) { - t.Errorf("Test %d: %s: Expected ETag to be not empty, but found empty instead (%v)", i+1, instanceType, result.Objects[j].Name) + } + // Since there are cases for which ListObjects fails, this is + // necessary. Test passes as expected, but the output values + // are verified for correctness here. + if err == nil && testCase.shouldPass { + // The length of the expected ListObjectsResult.Objects + // should match in both expected result from test cases + // and in the output. On failure calling t.Fatalf, + // otherwise it may lead to index out of range error in + // assertion following this. + if len(testCase.result.Objects) != len(result.Objects) { + t.Fatalf("Test %d: %s: Expected number of object in the result to be '%d', but found '%d' objects instead", i+1, instanceType, len(testCase.result.Objects), len(result.Objects)) } + for j := 0; j < len(testCase.result.Objects); j++ { + if testCase.result.Objects[j].Name != result.Objects[j].Name { + t.Errorf("Test %d: %s: Expected object name to be \"%s\", but found \"%s\" instead", i+1, instanceType, testCase.result.Objects[j].Name, result.Objects[j].Name) + } + // FIXME: we should always check for ETag + if result.Objects[j].ETag == "" && !strings.HasSuffix(result.Objects[j].Name, slashSeparator) { + t.Errorf("Test %d: %s: Expected ETag to be not empty, but found empty instead (%v)", i+1, instanceType, result.Objects[j].Name) + } - } - if testCase.result.IsTruncated != result.IsTruncated { - t.Errorf("Test %d: %s: Expected IsTruncated flag to be %v, but instead found it to be %v", i+1, instanceType, testCase.result.IsTruncated, result.IsTruncated) - } + } + if testCase.result.IsTruncated != result.IsTruncated { + t.Errorf("Test %d: %s: Expected IsTruncated flag to be %v, but instead found it to be %v", i+1, instanceType, testCase.result.IsTruncated, result.IsTruncated) + } - if testCase.result.IsTruncated && result.NextMarker == "" { - t.Errorf("Test %d: %s: Expected NextContinuationToken to contain a string since listing is truncated, but instead found it to be empty", i+1, instanceType) - } + if testCase.result.IsTruncated && result.NextMarker == "" { + t.Errorf("Test %d: %s: Expected NextContinuationToken to contain a string since listing is truncated, but instead found it to be empty", i+1, instanceType) + } - if !testCase.result.IsTruncated && result.NextMarker != "" { - t.Errorf("Test %d: %s: Expected NextContinuationToken to be empty since listing is not truncated, but instead found `%v`", i+1, instanceType, result.NextMarker) - } + if !testCase.result.IsTruncated && result.NextMarker != "" { + t.Errorf("Test %d: %s: Expected NextContinuationToken to be empty since listing is not truncated, but instead found `%v`", i+1, instanceType, result.NextMarker) + } - } - // Take ListObject treeWalk go-routine to completion, if available in the treewalk pool. - if result.IsTruncated { - _, err = obj.ListObjects(context.Background(), testCase.bucketName, testCase.prefix, result.NextMarker, testCase.delimeter, 1000) - if err != nil { - t.Fatal(err) } - } + // Take ListObject treeWalk go-routine to completion, if available in the treewalk pool. + if result.IsTruncated { + _, err = obj.ListObjects(context.Background(), testCase.bucketName, + testCase.prefix, result.NextMarker, testCase.delimeter, 1000) + if err != nil { + t.Fatal(err) + } + } + }) } } diff --git a/cmd/tree-walk.go b/cmd/tree-walk.go index 2f946159e..73b93e222 100644 --- a/cmd/tree-walk.go +++ b/cmd/tree-walk.go @@ -28,36 +28,6 @@ type TreeWalkResult struct { end bool } -// posix.ListDir returns entries with trailing "/" for directories. At the object layer -// we need to remove this trailing "/" for objects and retain "/" for prefixes before -// sorting because the trailing "/" can affect the sorting results for certain cases. -// Ex. lets say entries = ["a-b/", "a/"] and both are objects. -// sorting with out trailing "/" = ["a", "a-b"] -// sorting with trailing "/" = ["a-b/", "a/"] -// Hence if entries[] does not have a case like the above example then isLeaf() check -// can be delayed till the entry is pushed into the TreeWalkResult channel. -// delayIsLeafCheck() returns true if isLeaf can be delayed or false if -// isLeaf should be done in listDir() -func delayIsLeafCheck(entries []string) bool { - for i, entry := range entries { - if i == len(entries)-1 { - break - } - // If any byte in the "entry" string is less than '/' then the - // next "entry" should not contain '/' at the same same byte position. - for j := 0; j < len(entry); j++ { - if entry[j] < '/' { - if len(entries[i+1]) > j { - if entries[i+1][j] == '/' { - return false - } - } - } - } - } - return true -} - // Return entries that have prefix prefixEntry. // Note: input entries are expected to be sorted. func filterMatchingPrefix(entries []string, prefixEntry string) []string { @@ -81,49 +51,15 @@ func filterMatchingPrefix(entries []string, prefixEntry string) []string { } end-- } + sort.Strings(entries[start:end]) return entries[start:end] } // ListDirFunc - "listDir" function of type listDirFunc returned by listDirFactory() - explained below. -type ListDirFunc func(bucket, prefixDir, prefixEntry string) (entries []string, delayIsLeaf bool) - -// IsLeafFunc - A function isLeaf of type isLeafFunc is used to detect if an entry is a leaf entry. There are four scenarios -// where isLeaf should behave differently: -// 1. FS backend object listing - isLeaf is true if the entry has a trailing "/" -// 2. FS backend multipart listing - isLeaf is true if the entry is a directory and contains uploads.json -// 3. XL backend object listing - isLeaf is true if the entry is a directory and contains xl.json -// 4. XL backend multipart listing - isLeaf is true if the entry is a directory and contains uploads.json -type IsLeafFunc func(string, string) bool - -// IsLeafDirFunc - A function isLeafDir of type isLeafDirFunc is used to detect if an entry represents an empty directory. -type IsLeafDirFunc func(string, string) bool - -// Note: input entries are expected to be sorted. -func filterListEntries(bucket, prefixDir string, entries []string, prefixEntry string, isLeaf IsLeafFunc) ([]string, bool) { - // Filter entries that have the prefix prefixEntry. - entries = filterMatchingPrefix(entries, prefixEntry) - - // Can isLeaf() check be delayed till when it has to be sent down the - // TreeWalkResult channel? - delayIsLeaf := delayIsLeafCheck(entries) - if delayIsLeaf { - return entries, true - } - - // isLeaf() check has to happen here so that trailing "/" for objects can be removed. - for i, entry := range entries { - if isLeaf(bucket, pathJoin(prefixDir, entry)) { - entries[i] = strings.TrimSuffix(entry, slashSeparator) - } - } - // Sort again after removing trailing "/" for objects as the previous sort - // does not hold good anymore. - sort.Strings(entries) - return entries, false -} +type ListDirFunc func(bucket, prefixDir, prefixEntry string) (entries []string) // treeWalk walks directory tree recursively pushing TreeWalkResult into the channel as and when it encounters files. -func doTreeWalk(ctx context.Context, bucket, prefixDir, entryPrefixMatch, marker string, recursive bool, listDir ListDirFunc, isLeaf IsLeafFunc, isLeafDir IsLeafDirFunc, resultCh chan TreeWalkResult, endWalkCh chan struct{}, isEnd bool) error { +func doTreeWalk(ctx context.Context, bucket, prefixDir, entryPrefixMatch, marker string, recursive bool, listDir ListDirFunc, resultCh chan TreeWalkResult, endWalkCh chan struct{}, isEnd bool) error { // Example: // if prefixDir="one/two/three/" and marker="four/five.txt" treeWalk is recursively // called with prefixDir="one/two/three/four/" and marker="five.txt" @@ -139,12 +75,7 @@ func doTreeWalk(ctx context.Context, bucket, prefixDir, entryPrefixMatch, marker } } - entries, delayIsLeaf := listDir(bucket, prefixDir, entryPrefixMatch) - // When isleaf check is delayed, make sure that it is set correctly here. - if delayIsLeaf && isLeaf == nil { - return errInvalidArgument - } - + entries := listDir(bucket, prefixDir, entryPrefixMatch) // For an empty list return right here. if len(entries) == 0 { return nil @@ -163,21 +94,12 @@ func doTreeWalk(ctx context.Context, bucket, prefixDir, entryPrefixMatch, marker } for i, entry := range entries { - var leaf, leafDir bool - pentry := pathJoin(prefixDir, entry) - // Decision to do isLeaf check was pushed from listDir() to here. - if delayIsLeaf { - leaf = isLeaf(bucket, pentry) - if leaf { - entry = strings.TrimSuffix(entry, slashSeparator) - } - } else { - leaf = !strings.HasSuffix(entry, slashSeparator) - } - if strings.HasSuffix(entry, slashSeparator) { - leafDir = isLeafDir(bucket, pentry) + leaf := !hasSuffix(pentry, slashSeparator) + var leafDir bool + if !leaf { + leafDir = false } isDir := !leafDir && !leaf @@ -208,7 +130,7 @@ func doTreeWalk(ctx context.Context, bucket, prefixDir, entryPrefixMatch, marker // true at the end of the treeWalk stream. markIsEnd := i == len(entries)-1 && isEnd if err := doTreeWalk(ctx, bucket, pentry, prefixMatch, markerArg, recursive, - listDir, isLeaf, isLeafDir, resultCh, endWalkCh, markIsEnd); err != nil { + listDir, resultCh, endWalkCh, markIsEnd); err != nil { return err } continue @@ -228,7 +150,7 @@ func doTreeWalk(ctx context.Context, bucket, prefixDir, entryPrefixMatch, marker } // Initiate a new treeWalk in a goroutine. -func startTreeWalk(ctx context.Context, bucket, prefix, marker string, recursive bool, listDir ListDirFunc, isLeaf IsLeafFunc, isLeafDir IsLeafDirFunc, endWalkCh chan struct{}) chan TreeWalkResult { +func startTreeWalk(ctx context.Context, bucket, prefix, marker string, recursive bool, listDir ListDirFunc, endWalkCh chan struct{}) chan TreeWalkResult { // Example 1 // If prefix is "one/two/three/" and marker is "one/two/three/four/five.txt" // treeWalk is called with prefixDir="one/two/three/" and marker="four/five.txt" @@ -250,7 +172,7 @@ func startTreeWalk(ctx context.Context, bucket, prefix, marker string, recursive marker = strings.TrimPrefix(marker, prefixDir) go func() { isEnd := true // Indication to start walking the tree with end as true. - doTreeWalk(ctx, bucket, prefixDir, entryPrefixMatch, marker, recursive, listDir, isLeaf, isLeafDir, resultCh, endWalkCh, isEnd) + doTreeWalk(ctx, bucket, prefixDir, entryPrefixMatch, marker, recursive, listDir, resultCh, endWalkCh, isEnd) close(resultCh) }() return resultCh diff --git a/cmd/tree-walk_test.go b/cmd/tree-walk_test.go index 50eeccfbf..6262ff02b 100644 --- a/cmd/tree-walk_test.go +++ b/cmd/tree-walk_test.go @@ -30,49 +30,6 @@ import ( // Fixed volume name that could be used across tests const volume = "testvolume" -// Test for delayIsLeafCheck. -func TestDelayIsLeafCheck(t *testing.T) { - testCases := []struct { - entries []string - delay bool - }{ - // Test cases where isLeaf check can't be delayed. - { - []string{"a-b/", "a/"}, - false, - }, - { - []string{"a%b/", "a/"}, - false, - }, - { - []string{"a-b-c", "a-b/"}, - false, - }, - - // Test cases where isLeaf check can be delayed. - { - []string{"a-b/", "aa/"}, - true, - }, - { - []string{"a", "a-b"}, - true, - }, - { - []string{"aaa", "bbb"}, - true, - }, - } - for i, testCase := range testCases { - expected := testCase.delay - got := delayIsLeafCheck(testCase.entries) - if expected != got { - t.Errorf("Test %d : Expected %t got %t", i+1, expected, got) - } - } -} - // Test for filterMatchingPrefix. func TestFilterMatchingPrefix(t *testing.T) { entries := []string{"a", "aab", "ab", "abbbb", "zzz"} @@ -128,11 +85,11 @@ func createNamespace(disk StorageAPI, volume string, files []string) error { // Test if tree walker returns entries matching prefix alone are received // when a non empty prefix is supplied. -func testTreeWalkPrefix(t *testing.T, listDir ListDirFunc, isLeaf IsLeafFunc, isLeafDir IsLeafDirFunc) { +func testTreeWalkPrefix(t *testing.T, listDir ListDirFunc) { // Start the tree walk go-routine. prefix := "d/" endWalkCh := make(chan struct{}) - twResultCh := startTreeWalk(context.Background(), volume, prefix, "", true, listDir, isLeaf, isLeafDir, endWalkCh) + twResultCh := startTreeWalk(context.Background(), volume, prefix, "", true, listDir, endWalkCh) // Check if all entries received on the channel match the prefix. for res := range twResultCh { @@ -143,11 +100,11 @@ func testTreeWalkPrefix(t *testing.T, listDir ListDirFunc, isLeaf IsLeafFunc, is } // Test if entries received on tree walk's channel appear after the supplied marker. -func testTreeWalkMarker(t *testing.T, listDir ListDirFunc, isLeaf IsLeafFunc, isLeafDir IsLeafDirFunc) { +func testTreeWalkMarker(t *testing.T, listDir ListDirFunc) { // Start the tree walk go-routine. prefix := "" endWalkCh := make(chan struct{}) - twResultCh := startTreeWalk(context.Background(), volume, prefix, "d/g", true, listDir, isLeaf, isLeafDir, endWalkCh) + twResultCh := startTreeWalk(context.Background(), volume, prefix, "d/g", true, listDir, endWalkCh) // Check if only 3 entries, namely d/g/h, i/j/k, lmn are received on the channel. expectedCount := 3 @@ -184,23 +141,13 @@ func TestTreeWalk(t *testing.T) { t.Fatal(err) } - isLeaf := func(volume, prefix string) bool { - return !hasSuffix(prefix, slashSeparator) - } - - isLeafDir := func(volume, prefix string) bool { - entries, listErr := disk.ListDir(volume, prefix, 1, "") - if listErr != nil { - return false - } - return len(entries) == 0 - } + listDir := listDirFactory(context.Background(), disk) - listDir := listDirFactory(context.Background(), isLeaf, disk) // Simple test for prefix based walk. - testTreeWalkPrefix(t, listDir, isLeaf, isLeafDir) + testTreeWalkPrefix(t, listDir) // Simple test when marker is set. - testTreeWalkMarker(t, listDir, isLeaf, isLeafDir) + testTreeWalkMarker(t, listDir) + err = os.RemoveAll(fsDir) if err != nil { t.Fatal(err) @@ -228,19 +175,7 @@ func TestTreeWalkTimeout(t *testing.T) { t.Fatal(err) } - isLeaf := func(volume, prefix string) bool { - return !hasSuffix(prefix, slashSeparator) - } - - isLeafDir := func(volume, prefix string) bool { - entries, listErr := disk.ListDir(volume, prefix, 1, "") - if listErr != nil { - return false - } - return len(entries) == 0 - } - - listDir := listDirFactory(context.Background(), isLeaf, disk) + listDir := listDirFactory(context.Background(), disk) // TreeWalk pool with 2 seconds timeout for tree-walk go routines. pool := NewTreeWalkPool(2 * time.Second) @@ -249,7 +184,7 @@ func TestTreeWalkTimeout(t *testing.T) { prefix := "" marker := "" recursive := true - resultCh := startTreeWalk(context.Background(), volume, prefix, marker, recursive, listDir, isLeaf, isLeafDir, endWalkCh) + resultCh := startTreeWalk(context.Background(), volume, prefix, marker, recursive, listDir, endWalkCh) params := listParams{ bucket: volume, @@ -313,9 +248,7 @@ func TestListDir(t *testing.T) { } // create listDir function. - listDir := listDirFactory(context.Background(), func(volume, prefix string) bool { - return !hasSuffix(prefix, slashSeparator) - }, disk1, disk2) + listDir := listDirFactory(context.Background(), disk1, disk2) // Create file1 in fsDir1 and file2 in fsDir2. disks := []StorageAPI{disk1, disk2} @@ -327,7 +260,7 @@ func TestListDir(t *testing.T) { } // Should list "file1" from fsDir1. - entries, _ := listDir(volume, "", "") + entries := listDir(volume, "", "") if len(entries) != 2 { t.Fatal("Expected the number of entries to be 2") } @@ -345,7 +278,7 @@ func TestListDir(t *testing.T) { } // Should list "file2" from fsDir2. - entries, _ = listDir(volume, "", "") + entries = listDir(volume, "", "") if len(entries) != 1 { t.Fatal("Expected the number of entries to be 1") } @@ -373,21 +306,8 @@ func TestRecursiveTreeWalk(t *testing.T) { t.Fatalf("Unable to create StorageAPI: %s", err) } - // Simple isLeaf check, returns true if there is no trailing "/" - isLeaf := func(volume, prefix string) bool { - return !hasSuffix(prefix, slashSeparator) - } - - isLeafDir := func(volume, prefix string) bool { - entries, listErr := disk1.ListDir(volume, prefix, 1, "") - if listErr != nil { - return false - } - return len(entries) == 0 - } - // Create listDir function. - listDir := listDirFactory(context.Background(), isLeaf, disk1) + listDir := listDirFactory(context.Background(), disk1) // Create the namespace. var files = []string{ @@ -461,13 +381,16 @@ func TestRecursiveTreeWalk(t *testing.T) { }}, } for i, testCase := range testCases { - for entry := range startTreeWalk(context.Background(), volume, - testCase.prefix, testCase.marker, testCase.recursive, - listDir, isLeaf, isLeafDir, endWalkCh) { - if _, found := testCase.expected[entry.entry]; !found { - t.Errorf("Test %d: Expected %s, but couldn't find", i+1, entry.entry) + testCase := testCase + t.Run(fmt.Sprintf("Test%d", i+1), func(t *testing.T) { + for entry := range startTreeWalk(context.Background(), volume, + testCase.prefix, testCase.marker, testCase.recursive, + listDir, endWalkCh) { + if _, found := testCase.expected[entry.entry]; !found { + t.Errorf("Expected %s, but couldn't find", entry.entry) + } } - } + }) } err = os.RemoveAll(fsDir1) if err != nil { @@ -488,21 +411,8 @@ func TestSortedness(t *testing.T) { t.Fatalf("Unable to create StorageAPI: %s", err) } - // Simple isLeaf check, returns true if there is no trailing "/" - isLeaf := func(volume, prefix string) bool { - return !hasSuffix(prefix, slashSeparator) - } - - isLeafDir := func(volume, prefix string) bool { - entries, listErr := disk1.ListDir(volume, prefix, 1, "") - if listErr != nil { - return false - } - return len(entries) == 0 - } - // Create listDir function. - listDir := listDirFactory(context.Background(), isLeaf, disk1) + listDir := listDirFactory(context.Background(), disk1) // Create the namespace. var files = []string{ @@ -544,7 +454,7 @@ func TestSortedness(t *testing.T) { var actualEntries []string for entry := range startTreeWalk(context.Background(), volume, test.prefix, test.marker, test.recursive, - listDir, isLeaf, isLeafDir, endWalkCh) { + listDir, endWalkCh) { actualEntries = append(actualEntries, entry.entry) } if !sort.IsSorted(sort.StringSlice(actualEntries)) { @@ -572,20 +482,8 @@ func TestTreeWalkIsEnd(t *testing.T) { t.Fatalf("Unable to create StorageAPI: %s", err) } - isLeaf := func(volume, prefix string) bool { - return !hasSuffix(prefix, slashSeparator) - } - - isLeafDir := func(volume, prefix string) bool { - entries, listErr := disk1.ListDir(volume, prefix, 1, "") - if listErr != nil { - return false - } - return len(entries) == 0 - } - // Create listDir function. - listDir := listDirFactory(context.Background(), isLeaf, disk1) + listDir := listDirFactory(context.Background(), disk1) // Create the namespace. var files = []string{ @@ -626,7 +524,8 @@ func TestTreeWalkIsEnd(t *testing.T) { } for i, test := range testCases { var entry TreeWalkResult - for entry = range startTreeWalk(context.Background(), volume, test.prefix, test.marker, test.recursive, listDir, isLeaf, isLeafDir, endWalkCh) { + for entry = range startTreeWalk(context.Background(), volume, test.prefix, + test.marker, test.recursive, listDir, endWalkCh) { } if entry.entry != test.expectedEntry { t.Errorf("Test %d: Expected entry %s, but received %s with the EOF marker", i, test.expectedEntry, entry.entry) diff --git a/cmd/xl-sets.go b/cmd/xl-sets.go index 099cc3953..ed68203b1 100644 --- a/cmd/xl-sets.go +++ b/cmd/xl-sets.go @@ -643,7 +643,7 @@ func (s *xlSets) CopyObject(ctx context.Context, srcBucket, srcObject, destBucke // Returns function "listDir" of the type listDirFunc. // isLeaf - is used by listDir function to check if an entry is a leaf or non-leaf entry. // disks - used for doing disk.ListDir(). Sets passes set of disks. -func listDirSetsFactory(ctx context.Context, isLeaf IsLeafFunc, isLeafDir IsLeafDirFunc, sets ...*xlObjects) ListDirFunc { +func listDirSetsFactory(ctx context.Context, sets ...*xlObjects) ListDirFunc { listDirInternal := func(bucket, prefixDir, prefixEntry string, disks []StorageAPI) (mergedEntries []string) { var diskEntries = make([][]string, len(disks)) var wg sync.WaitGroup @@ -684,7 +684,7 @@ func listDirSetsFactory(ctx context.Context, isLeaf IsLeafFunc, isLeafDir IsLeaf } // listDir - lists all the entries at a given prefix and given entry in the prefix. - listDir := func(bucket, prefixDir, prefixEntry string) (mergedEntries []string, delayIsLeaf bool) { + listDir := func(bucket, prefixDir, prefixEntry string) (mergedEntries []string) { for _, set := range sets { var newEntries []string // Find elements in entries which are not in mergedEntries @@ -703,7 +703,7 @@ func listDirSetsFactory(ctx context.Context, isLeaf IsLeafFunc, isLeafDir IsLeaf sort.Strings(mergedEntries) } } - return filterListEntries(bucket, prefixDir, mergedEntries, prefixEntry, isLeaf) + return filterMatchingPrefix(mergedEntries, prefixEntry) } return listDir } @@ -712,15 +712,7 @@ func listDirSetsFactory(ctx context.Context, isLeaf IsLeafFunc, isLeafDir IsLeaf // listed and subsequently merge lexically sorted inside listDirSetsFactory(). Resulting // value through the walk channel receives the data properly lexically sorted. func (s *xlSets) ListObjects(ctx context.Context, bucket, prefix, marker, delimiter string, maxKeys int) (ListObjectsInfo, error) { - isLeaf := func(bucket, entry string) bool { - return !hasSuffix(entry, slashSeparator) - } - - isLeafDir := func(bucket, entry string) bool { - return false - } - - listDir := listDirSetsFactory(ctx, isLeaf, isLeafDir, s.sets...) + listDir := listDirSetsFactory(ctx, s.sets...) var getObjectInfoDirs []func(context.Context, string, string) (ObjectInfo, error) // Verify prefixes in all sets. @@ -732,7 +724,7 @@ func (s *xlSets) ListObjects(ctx context.Context, bucket, prefix, marker, delimi return s.getHashedSet(entry).getObjectInfo(ctx, bucket, entry) } - return listObjects(ctx, s, bucket, prefix, marker, delimiter, maxKeys, s.listPool, isLeaf, isLeafDir, listDir, getObjectInfo, getObjectInfoDirs...) + return listObjects(ctx, s, bucket, prefix, marker, delimiter, maxKeys, s.listPool, listDir, getObjectInfo, getObjectInfoDirs...) } func (s *xlSets) ListMultipartUploads(ctx context.Context, bucket, prefix, keyMarker, uploadIDMarker, delimiter string, maxUploads int) (result ListMultipartsInfo, err error) { @@ -1245,16 +1237,8 @@ func (s *xlSets) HealObjects(ctx context.Context, bucket, prefix string, healObj recursive := true endWalkCh := make(chan struct{}) - isLeaf := func(bucket, entry string) bool { - return hasSuffix(entry, xlMetaJSONFile) - } - - isLeafDir := func(bucket, entry string) bool { - return false - } - - listDir := listDirSetsFactory(ctx, isLeaf, isLeafDir, s.sets...) - walkResultCh := startTreeWalk(ctx, bucket, prefix, "", recursive, listDir, isLeaf, isLeafDir, endWalkCh) + listDir := listDirSetsFactory(ctx, s.sets...) + walkResultCh := startTreeWalk(ctx, bucket, prefix, "", recursive, listDir, endWalkCh) for { walkResult, ok := <-walkResultCh if !ok { diff --git a/cmd/xl-v1-common.go b/cmd/xl-v1-common.go index 16203e819..65c1c903d 100644 --- a/cmd/xl-v1-common.go +++ b/cmd/xl-v1-common.go @@ -85,34 +85,3 @@ func (xl xlObjects) isObject(bucket, prefix string) (ok bool) { return reduceReadQuorumErrs(context.Background(), errs, objectOpIgnoredErrs, readQuorum) == nil } - -// isObjectDir returns if the specified path represents an empty directory. -func (xl xlObjects) isObjectDir(bucket, prefix string) (ok bool) { - var errs = make([]error, len(xl.getDisks())) - var wg sync.WaitGroup - for index, disk := range xl.getDisks() { - if disk == nil { - continue - } - wg.Add(1) - go func(index int, disk StorageAPI) { - defer wg.Done() - // Check if 'prefix' is an object on this 'disk', else continue the check the next disk - entries, err := disk.ListDir(bucket, prefix, 1, "") - if err != nil { - errs[index] = err - return - } - if len(entries) > 0 { - errs[index] = errVolumeNotEmpty - return - } - }(index, disk) - } - - wg.Wait() - - readQuorum := len(xl.getDisks()) / 2 - - return reduceReadQuorumErrs(context.Background(), errs, objectOpIgnoredErrs, readQuorum) == nil -} diff --git a/cmd/xl-v1-list-objects.go b/cmd/xl-v1-list-objects.go index 2664e1058..6083dda02 100644 --- a/cmd/xl-v1-list-objects.go +++ b/cmd/xl-v1-list-objects.go @@ -22,11 +22,10 @@ import ( ) // Returns function "listDir" of the type listDirFunc. -// isLeaf - is used by listDir function to check if an entry is a leaf or non-leaf entry. // disks - used for doing disk.ListDir() -func listDirFactory(ctx context.Context, isLeaf IsLeafFunc, disks ...StorageAPI) ListDirFunc { +func listDirFactory(ctx context.Context, disks ...StorageAPI) ListDirFunc { // Returns sorted merged entries from all the disks. - listDir := func(bucket, prefixDir, prefixEntry string) (mergedEntries []string, delayIsLeaf bool) { + listDir := func(bucket, prefixDir, prefixEntry string) (mergedEntries []string) { for _, disk := range disks { if disk == nil { continue @@ -55,8 +54,7 @@ func listDirFactory(ctx context.Context, isLeaf IsLeafFunc, disks ...StorageAPI) sort.Strings(mergedEntries) } } - mergedEntries, delayIsLeaf = filterListEntries(bucket, prefixDir, mergedEntries, prefixEntry, isLeaf) - return mergedEntries, delayIsLeaf + return filterMatchingPrefix(mergedEntries, prefixEntry) } return listDir } @@ -72,10 +70,8 @@ func (xl xlObjects) listObjects(ctx context.Context, bucket, prefix, marker, del walkResultCh, endWalkCh := xl.listPool.Release(listParams{bucket, recursive, marker, prefix}) if walkResultCh == nil { endWalkCh = make(chan struct{}) - isLeaf := xl.isObject - isLeafDir := xl.isObjectDir - listDir := listDirFactory(ctx, isLeaf, xl.getLoadBalancedDisks()...) - walkResultCh = startTreeWalk(ctx, bucket, prefix, marker, recursive, listDir, isLeaf, isLeafDir, endWalkCh) + listDir := listDirFactory(ctx, xl.getLoadBalancedDisks()...) + walkResultCh = startTreeWalk(ctx, bucket, prefix, marker, recursive, listDir, endWalkCh) } var objInfos []ObjectInfo