[REEF-2022] Fixed AzureBlockBlobFileSystem.GetChildren api and unit tests #1464

sharathmalladi · 2018-05-22T03:03:12Z

Fixed the GetChildren api and added unit test.

JIRA:
[REEF-2022] https://issues.apache.org/jira/browse/REEF-2022

tyclintw · 2018-05-22T20:05:28Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

@@ -162,13 +162,27 @@ public void DeleteDirectory(Uri directoryUri)
        public IEnumerable<Uri> GetChildren(Uri directoryUri)
        {
            BlobContinuationToken blobContinuationToken = null;
-            var path = directoryUri.AbsolutePath.Trim('/');
+            string path = directoryUri.AbsolutePath.Trim('/');


This path work is unnecessary. Just use directoryUri.Segments #Closed

Done. Thanks for the suggestion. #Resolved

tyclintw · 2018-05-22T20:07:38Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/ICloudBlobClient.cs

        /// </summary>
-        BlobResultSegment ListBlobsSegmented(string prefix, bool useFlatListing, BlobListingDetails blobListingDetails, int? maxResults, 
+        BlobResultSegment ListBlobsSegmented(string containerName, string directoryName, bool useFlatListing, BlobListingDetails blobListingDetails, int? maxResults, 


"directoryName" is confusing since this could actually be a path. I suggest using "relativeAddress" to match the blob API. #Closed

Done. #Resolved

tyclintw · 2018-05-22T20:12:39Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+
+            Assert.Equal(allNames.Count(), blobs.Count());
+
+            // List files only in the sub-folder in the container


The second sections of this test is identical to the first section of this test. Please consolidate to a helper method. #Closed

Consolidated code into a common method. #Resolved

tyclintw · 2018-05-22T20:14:36Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

@@ -138,6 +139,43 @@ public void TestDeleteE2E()
            Assert.False(CheckBlobExists(blob));
        }

+        [Fact(Skip = SkipMessage)]
+        public void TestGetChildrenE2E()


We should also test listing containers. After all the IFileSystem should work like the local file system. Containers are just root directories from an outside observer. #Closed

Good idea! I added code to make it work for both containers and blobs. #Closed

tyclintw · 2018-05-23T17:12:35Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            Assert.Equal(expectedChildBlobNames.Count(), blobNames.Count());
+        }
+
+        public void ValidateChildrenWithContainers(Uri rootUri, IEnumerable<string> expectedContainerNames)


No reason for another method. This method is basically identical to ValidateChildenWithBlobs. #Closed

Hmm .. they are different .. in how we extract out the blob names or container names as the case may be from the output uri. I could try to consolidate it even more but I am afraid that we lose readability (containers vs blobs). Lets chat offline if you are not convinced with the reasoning. #Resolved

Fixed to remove the duplicate method. #Resolved

tyclintw · 2018-05-23T17:13:18Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+
+        public void ValidateChildenWithBlobs(Uri storageBlobUri, IEnumerable<string> expectedChildBlobNames)
+        {
+            IEnumerable<Uri> blobs = _fileSystem.GetChildren(storageBlobUri).ToList();


ToList() is unnecessary. #Closed

Right. Got rid of it. #Resolved

tyclintw · 2018-05-23T17:15:49Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            IEnumerable<Uri> blobs = _fileSystem.GetChildren(storageBlobUri).ToList();
+            IEnumerable<string> blobNames = blobs.Select(b => b.LocalPath.Replace("/" + _container.Name + "/", string.Empty));
+
+            Assert.True(expectedChildBlobNames.All(blobName => blobNames.Contains(blobName)), "blobNames has elements that are not in expectedChildBlobNames");


CollectionAssert.AreEquivalent should be used instead. #Closed

I dont see AreEquivalent. I do see Assert.Equal() here but that requires that the two lists be in the same order since it uses CheckIfEnumerablesAreEqual .. which is not guaranteed in the lists that I have. I could go ahead and sort and then do this, but I felt that what I have is simple enough to follow. #Resolved

tyclintw · 2018-05-23T17:16:10Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            IEnumerable<string> blobNames = blobs.Select(b => b.LocalPath.Replace("/" + _container.Name + "/", string.Empty));
+
+            Assert.True(expectedChildBlobNames.All(blobName => blobNames.Contains(blobName)), "blobNames has elements that are not in expectedChildBlobNames");
+            Assert.Equal(expectedChildBlobNames.Count(), blobNames.Count());


Unnecessary if CollectionAssert is used. #Closed

tyclintw · 2018-05-23T17:19:18Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

+            }
+
+            // If not at the root folder, return all blobs within the container
+            string containerName = segments[1];


Possible out of bounds exception. #Closed

Fixed by changing the condition earlier by ensuring this line is not executed if segments.Count() <= 1. #Resolved

tyclintw · 2018-05-23T17:38:56Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            foreach (string uploadedBlobName in parentFileNames.Concat(folderfileNames))
+            {
+                CloudBlockBlob blob = _container.GetBlockBlobReference(uploadedBlobName);
+                UploadFromString(blob, "hello");


Are these files cleaned up anywhere if the test fails and you want to rerun? #Closed

Yes, the Dispose() method cleans up the container and everything in it. #Resolved

tyclintw · 2018-05-23T17:59:05Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            // setup
+            string[] parentFileNames = new string[] { "sample1", "sample2", "sample3" };
+            string[] folderfileNames = new string[] { "folder1/sample4", "folder1/sample5" };
+            string[] parentListNames = new string[] { "sample1", "sample2", "sample3", "folder1/" };


These three arrays and their naming conventions make if very difficult to follow your logic. Can this be simplified with one list and the use of select statements? Something like the following. At the very least change parentFileNames to a concrete list so the concat at 150 isn't necessary.

string[] fileNames = new string[] { "sample1", "sample2", "sample3", "folder1/sample4", "folder1/sample5" }; var rootFilenames = fileNames.GroupBy(f => f.Split('/')[0]).Select(g => g.Key); ValidateChildenWithBlobs(_container.Uri, rootFilenames); var folderBlobs = fileNames.Where(f => f.StartsWith("folder1"); ValidateChildenWithBlobs(folderUri, folderBlobs);

OR

string[] fileNames = new string[] { "sample1", "sample2", "sample3","folder1/sample4", "folder1/sample5" }; string[] expectedFolderChildren = new string[] { "folder1/sample4", "folder1/sample5" }; string[] expectedRootChildren = new string[] { "sample1", "sample2", "sample3", "folder1/" }; ``` #Closed

OK. Took the second option that you proposed. #Resolved

tyclintw · 2018-05-23T23:26:33Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

+            // If not at the root folder, return all blobs within the container
+            string containerName = segments[1];
+            string relativeAddress = string.Empty;
+            if (segments.Count() > 1)


unnecessary check since we've already guaranteed length >= 1. #Closed

Removed check. #Resolved

tyclintw · 2018-05-24T20:28:18Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+        {
+            IEnumerable<Uri> blobs = _fileSystem.GetChildren(storageBlobUri);
+            Assert.Equal(
+                expectedChildBlobs.Select(uri => uri.AbsoluteUri).OrderBy(uri => uri),


Select shouldn't necessasry #Closed

System.Uri does not implement IComparer. So I had to get the AbsoluteUri (string) before doing the OrderBy. #Resolved

sharathmalladi · 2018-05-24T23:35:20Z

@markusweimer and @motus : This pull request has been approved. Please review as needed and merge to master. Thanks!

…xed build issues

sharathmalladi · 2018-06-05T23:40:17Z

Pinging again for @markusweimer or @motus to please merge this pull request.

The test failures that I see with the appveyor failures seem unrelated to this pull request.

motus · 2018-06-06T00:01:25Z

@sharathmalladi please give me a day or two. I really want to review and merge it, but I am in the middle of the huge #1466 now

sharathmalladi · 2018-06-15T17:53:34Z

Pinging again for @markusweimer or @motus to please merge this pull request. There are other changes that are piling up that depend on this one.

motus

Looks pretty good! I am done with my round of nit picks 😄

P.S. a general comment: try to keep lines shorter than 120 characters (preferably below 100); trim trailing whitespaces; use CodeMaid or similar tool to fix these and other formatting issues for you

motus · 2018-06-19T23:45:36Z

lang/cs/Org.Apache.REEF.Driver/Evaluator/EvaluatorRequest.cs

@@ -29,7 +29,7 @@ namespace Org.Apache.REEF.Driver.Evaluator
    internal class EvaluatorRequest : IEvaluatorRequest
    {
        internal EvaluatorRequest()
-            : this(0, 0, 1, string.Empty, Guid.NewGuid().ToString("N"), string.Empty, Enumerable.Empty<string>().ToList(), true, String.Empty)
+            : this(0, 0, 1, string.Empty, Guid.NewGuid().ToString("N"), string.Empty, Enumerable.Empty<string>().ToList(), true, string.Empty)


By the way, why is it so complicated, with Enumerable and such? Can't we just write, say

private static readonly string NoBatchId = Guid.NewGuid().ToString("N"); private static readonly string[] NoNodeNames = { }; internal EvaluatorRequest() : this(0, 0, 1, "", NoBatchId, "", NoNodeNames, true, "")

or, if you want more readability, maybe

internal EvaluatorRequest() : this( number: 0, megaBytes: 0, core: 1, rack: "", NoBatchId, runtimeName: "", NoNodeNames, relaxLocality: true, nodeLabelExpression: "")

etc

Refined constructors by calling the relevant overrides in each place.

In reply to: 196612445 [](ancestors = 196612445)

motus · 2018-06-20T04:22:39Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            // setup
+            string[] fileNames = new string[] { "sample1", "sample2", "sample3", "folder1/sample4", "folder1/sample5" };
+            string[] expectedFolderChildren = new string[] { "folder1/sample4", "folder1/sample5" };
+            string[] expectedRootChildren = new string[] { "sample1", "sample2", "sample3", "folder1/" };


string arrays can be private readonly

C# local variables cannot be marked readonly. If the suggestion was to make these as fields of the TestAzureBlockBlobFileSystemE2E class instead: I feel like having to expand the scope just to make it readonly is not a great idea. Wont fix.

In reply to: 196644714 [](ancestors = 196644714)

motus · 2018-06-20T05:04:15Z

lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs

+            ValidateChildren(rootUri, new List<Uri> { _container.Uri });
+        }
+
+        public void ValidateChildren(Uri storageBlobUri, IEnumerable<Uri> expectedChildBlobs)


can be private

Fixed.

In reply to: 196649314 [](ancestors = 196649314)

motus · 2018-06-20T05:23:53Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

@@ -157,22 +157,58 @@ public void DeleteDirectory(Uri directoryUri)
        }


maybe

Task.WaitAll(directory .ListBlobs(true) .OfType<ICloudBlob>() .Select(blob => blob.DeleteIfExistsAsync()) .ToArray());

Parallelization is better. Thanks! Fixed.

In reply to: 196651685 [](ancestors = 196651685)

motus · 2018-06-20T05:27:46Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

-            var path = directoryUri.AbsolutePath.Trim('/');
+
+            // If at the root, return all containers
+            if (segments.Count() <= 1)


better use segments.Length instead of Linq

Fixed.

In reply to: 196652155 [](ancestors = 196652155)

motus · 2018-06-20T05:31:26Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

+                        }
+                    }
+
+                    blobContinuationToken = containerListing.ContinuationToken;


will raise an NPE if containerListing can ever be null. did you mean ?. ?

Fixed.

In reply to: 196652608 [](ancestors = 196652608)

motus · 2018-06-20T05:36:13Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

+                {
+                    throw new ArgumentException("Call to ListBlobsSegmented returned no results. Uri is invalid or does not have children.", nameof(directoryUri));
+                }
+                else


no need for else block

Fixed. All files that were touched in this pr now have less than 120 characters per line.

In reply to: 196653152 [](ancestors = 196653152)

motus · 2018-06-20T05:51:03Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs

-                if (listing.Results != null)
+                if (listing == null || listing.Results.Count() == 0)
+                {
+                    throw new ArgumentException("Call to ListBlobsSegmented returned no results. Uri is invalid or does not have children.", nameof(directoryUri));


this line is quite long. our coding standard requires that lines should be no longer than 120 characters; we usually keep it below 100 for readability.

motus · 2018-06-20T05:52:48Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureCloudBlobClient.cs

+                maxResults,
+                continuationToken,
+                blobRequestOptions,
+                operationContext).GetAwaiter().GetResult();


.GetAwaiter().GetResult() can be simply .Result

I see some discussions on this topic and it appears the two have different responses if an exception is thrown: https://stackoverflow.com/questions/17284517/is-task-result-the-same-as-getawaiter-getresult

From that discussion, I wager that GetAwaiter().GetResult() is the better option to use. If you still disagree, we can take this offline and resolve this.

In reply to: 196655236 [](ancestors = 196655236)

good point!

motus · 2018-06-20T05:52:56Z

lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureCloudBlobClient.cs

+
+        public ContainerResultSegment ListContainersSegmented(BlobContinuationToken continuationToken)
+        {
+            return _client.ListContainersSegmentedAsync(continuationToken).GetAwaiter().GetResult();


same as above

Replied on the other issue and I will follow the same resolution here once that is addressed.

In reply to: 196655252 [](ancestors = 196655252)

motus

Neat!! Will test and merge now

tyclintw suggested changes May 22, 2018

View reviewed changes

tyclintw suggested changes May 23, 2018

View reviewed changes

tyclintw reviewed May 23, 2018

View reviewed changes

tyclintw approved these changes May 24, 2018

View reviewed changes

sharathmalladi added 5 commits May 31, 2018 17:39

Fixed AzureBlockBlobFileSystem.GetChildren api and unit tests

0ed055f

Addressed feedback comments

da567a0

Addressed more feedback comments

4d11540

Removed duplicate helper method

49653af

Fixed blob filesystem to align with local file system behavior and fi…

d8d82da

…xed build issues

sharathmalladi force-pushed the users/sharathm/blobgetchildren branch from 9d4cb16 to d8d82da Compare June 1, 2018 00:45

motus suggested changes Jun 20, 2018

View reviewed changes

Reacting to code review comments

d120294

motus approved these changes Jun 20, 2018

View reviewed changes

motus merged commit 396fd82 into apache:master Jun 20, 2018


		Assert.Equal(allNames.Count(), blobs.Count());

		// List files only in the sub-folder in the container

		@@ -157,22 +157,58 @@ public void DeleteDirectory(Uri directoryUri)
		}

[REEF-2022] Fixed AzureBlockBlobFileSystem.GetChildren api and unit tests #1464

[REEF-2022] Fixed AzureBlockBlobFileSystem.GetChildren api and unit tests #1464

Conversation

sharathmalladi commented May 22, 2018

tyclintw May 22, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 22, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 22, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 22, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 24, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 23, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 23, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 24, 2018 • edited

Choose a reason for hiding this comment

tyclintw May 24, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi May 24, 2018 • edited

Choose a reason for hiding this comment

sharathmalladi commented May 24, 2018

sharathmalladi commented Jun 5, 2018 • edited

motus commented Jun 6, 2018 • edited

sharathmalladi commented Jun 15, 2018

motus 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

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

sharathmalladi Jun 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharathmalladi Jun 20, 2018 • edited

tyclintw May 22, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 22, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 22, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 22, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

sharathmalladi May 24, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 23, 2018 •

edited

tyclintw May 23, 2018 •

edited

sharathmalladi May 24, 2018 •

edited

tyclintw May 24, 2018 •

edited

sharathmalladi May 24, 2018 •

edited

sharathmalladi commented Jun 5, 2018 •

edited

motus commented Jun 6, 2018 •

edited

sharathmalladi Jun 20, 2018 •

edited

sharathmalladi Jun 20, 2018 •

edited