Skip to content
This repository was archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-2036] Add IFileSystem api to check if uri is a directory #1473

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[REEF-2036] Add IFileSystem api to check if uri is a directory #1473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d081df9
Add IFileSystem api to check if uri is a directory
dwaijam Jun 9, 2018
5d8189a
address pr comments
dwaijam Jun 27, 2018
272feaa
clean up
dwaijam Jun 27, 2018
e7f44e3
address PR comments
dwaijam Jun 30, 2018
ff4ad02
Address PR comments
dwaijam Jun 30, 2018
7e443a6
Address pr comments
dwaijam Jun 30, 2018
23bf594
Address pr comments
dwaijam Jun 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 62 additions & 42 deletions lang/cs/Org.Apache.REEF.IO.Tests/TestAzureBlockBlobFileSystemE2E.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@ namespace Org.Apache.REEF.IO.Tests
/// </summary>
public sealed class TestAzureBlockBlobFileSystemE2E : IDisposable
{
private const string SkipMessage = "Fill in credentials before running test"; // Use null to run tests
// Uncomment SkipMessage = null to run tests
private const string SkipMessage = "Fill in credentials before running test";
// private const string SkipMessage = null;
private const string HelloFile = "hello";
private IFileSystem _fileSystem;
private CloudBlobClient _client;
private CloudBlobContainer _container;
private readonly IFileSystem _fileSystem;
private readonly CloudBlobClient _client;
private readonly CloudBlobContainer _container;

public TestAzureBlockBlobFileSystemE2E()
{
Expand All @@ -66,26 +68,22 @@ public void Dispose()

private bool CheckBlobExists(ICloudBlob blob)
{
var task = blob.ExistsAsync();
return task.Result;
return blob.ExistsAsync().GetAwaiter().GetResult();
}

private bool CheckContainerExists(CloudBlobContainer container)
{
var task = container.ExistsAsync();
return task.Result;
return container.ExistsAsync().GetAwaiter().GetResult();
}

private ICloudBlob GetBlobReferenceFromServer(CloudBlobContainer container, string blobName)
{
var task = container.GetBlobReferenceFromServerAsync(blobName);
return task.Result;
return container.GetBlobReferenceFromServerAsync(blobName).GetAwaiter().GetResult();
}

private string DownloadText(CloudBlockBlob blob)
{
var task = blob.DownloadTextAsync();
return task.Result;
return blob.DownloadTextAsync().GetAwaiter().GetResult();
}

[Fact(Skip = SkipMessage)]
Expand Down Expand Up @@ -161,9 +159,9 @@ public void TestGetChildBlobsInContainerE2E()
[Fact(Skip = SkipMessage)]
public void TestGetChildContainerInStorageAccountE2E()
{
// List containers in the storage account
Uri rootUri = _fileSystem.CreateUriForPath(string.Empty);
ValidateChildren(rootUri, new List<Uri> { _container.Uri });
// List container uris in the storage account
var containerUris = _client.ListContainers().Select(container => container.Uri);
ValidateChildren(_client.BaseUri, containerUris);
}

private void ValidateChildren(Uri storageBlobUri, IEnumerable<Uri> expectedChildBlobs)
Expand Down Expand Up @@ -264,6 +262,37 @@ public void TestCopyFromLocalE2E()
container.DeleteIfExistsAsync().Wait();
}

[Fact(Skip = SkipMessage)]
Copy link
Contributor

@singlis singlis Jun 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I know this is following the existing pattern -- but instead of having to remove all the skips and then set credentials, can we change this to dynamically skip the test if the credentials are not set? It looks like xunit does support a SkipTestException: https://github.com/xunit/samples.xunit/blob/master/DynamicSkipExample/SkipTestException.cs

Not sure if this is supported in our version of xunit or not. #Resolved

Copy link
Contributor

@singlis singlis Jun 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I would also put the check in a single function and call it before the test. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SkippableFact is not supported in our version of xunit.
Yopu dont need to change each SkipMessage.Just set SkipMessage to null at top and all tests will run. These tests can only be run locally and a valid connectionstring is required to be inserted. I dont see how making this dynamic would be any help since the conn string validation will be run everytime in a build pipeline and it will always be expected to fail

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This value can only be a const string.
You dont need to change each SkipMessage.Just set SkipMessage to null at top and all tests will run. These tests can only be run locally and a valid connectionstring is required to be inserted. I dont see how making this dynamic would be any help since the conn string validation will be run everytime in a build pipeline and it will always be expected to fail

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

Copy link
Contributor

@singlis singlis Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

OK sounds good, it maybe worth making that a bit more clear above ( there is a comment off to the side that says use null to run tests. It maybe more clear to have another line under SkipMessage with SkipMessage = null commented out and a comment above to uncomment to run skipped tests.

In reply to: 196930627 [](ancestors = 196930627,195885915)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this as it is as discussed offline

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please elaborate? We might need to refer to this PR in a future when we have all forgotten what was discussed offline :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scott said it is fine to have the current line.
private const string SkipMessage = "Fill in credentials before running test"; // Use null to run tests

Copy link
Contributor

@singlis singlis Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I did -- so setting SkipMessage=null has the same effect as the SkipTestException since our version of XUnit does not support it. But I think the comment of "// Use null to run tests" could be a bit more explicit. This is more what I was thinking:

SkipMessage="....";
//Uncomment SkipMessage=null to run tests
//SkipMessage=null
``` #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested

public void TestIsDirectoryValidDirectoryLevel1E2E()
{
const string Directory = "dir";
CreateTempBlobs(Directory);
Assert.True(_fileSystem.IsDirectory(PathToFile(Directory)));
}

[Fact(Skip = SkipMessage)]
public void TestIsDirectoryValidDirectoryLevel2E2E()
{
const string Directory = "dir1/dir2";
CreateTempBlobs(Directory);
Assert.True(_fileSystem.IsDirectory(PathToFile(Directory)));
}

[Fact(Skip = SkipMessage)]
public void TestIsDirectoryFakeDirectoryE2E()
{
const string Directory = "dir";
Assert.False(_fileSystem.IsDirectory(PathToFile(Directory)));
}

[Fact(Skip = SkipMessage)]
public void TestIsDirectoryFileE2E()
{
const string Directory = "dir";
var blockBlobs = CreateTempBlobs(Directory);
Assert.False(_fileSystem.IsDirectory(blockBlobs.First().Uri));
}

[Fact(Skip = SkipMessage)]
public void TestDeleteDirectoryAtContainerE2E()
{
Expand All @@ -274,16 +303,8 @@ public void TestDeleteDirectoryAtContainerE2E()
[Fact(Skip = SkipMessage)]
public void TestDeleteDirectoryFirstLevelE2E()
{
const string Directory = "dir";
var blockBlobs = new List<CloudBlockBlob>();
for (var i = 0; i < 3; i++)
{
var filePath = Directory + '/' + i;
var blockBlob = _container.GetBlockBlobReference(filePath);
UploadFromString(blockBlob, "hello");
Assert.True(CheckBlobExists(blockBlob));
blockBlobs.Add(blockBlob);
}
const string Directory = "dir1";
var blockBlobs = CreateTempBlobs(Directory);

_fileSystem.DeleteDirectory(PathToFile(Directory));

Expand All @@ -299,24 +320,11 @@ public void TestDeleteDirectoryFirstLevelE2E()
public void TestDeleteDirectorySecondLevelE2E()
{
const string Directory1 = "dir1";
const string Directory2 = "dir2";
var blockBlobs1 = new List<CloudBlockBlob>();
var blockBlobs2 = new List<CloudBlockBlob>();
for (var i = 0; i < 3; i++)
{
var filePath1 = Directory1 + '/' + i;
var filePath2 = Directory1 + '/' + Directory2 + '/' + i;
var blockBlob1 = _container.GetBlockBlobReference(filePath1);
var blockBlob2 = _container.GetBlockBlobReference(filePath2);
UploadFromString(blockBlob1, "hello");
UploadFromString(blockBlob2, "hello");
Assert.True(CheckBlobExists(blockBlob1));
Assert.True(CheckBlobExists(blockBlob2));
blockBlobs1.Add(blockBlob1);
blockBlobs2.Add(blockBlob2);
}
const string Directory2 = "dir1/dir2";
var blockBlobs1 = CreateTempBlobs(Directory1);
var blockBlobs2 = CreateTempBlobs(Directory2);

_fileSystem.DeleteDirectory(PathToFile(Directory1 + '/' + Directory2));
_fileSystem.DeleteDirectory(PathToFile(Directory2));

foreach (var blockBlob in blockBlobs2)
{
Expand All @@ -331,6 +339,18 @@ public void TestDeleteDirectorySecondLevelE2E()
Assert.True(CheckContainerExists(_container));
}

private IEnumerable<CloudBlockBlob> CreateTempBlobs(string directory, int fileCount = 3)
{
return Enumerable.Range(0, fileCount).Select(i =>
{
var filePath = directory + '/' + i;
var blockBlob = _container.GetBlockBlobReference(filePath);
UploadFromString(blockBlob, "hello");
Assert.True(CheckBlobExists(blockBlob), "Blob does not exist: " + filePath);
return blockBlob;
});
}

private static void UploadFromString(ICloudBlob blob, string str)
{
var byteArray = Encoding.UTF8.GetBytes(str);
Expand Down
22 changes: 22 additions & 0 deletions lang/cs/Org.Apache.REEF.IO.Tests/TestAzureDataLakeFileSystemE2E.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,28 @@ public void TestCreateDirectoryE2E()
Assert.True(_adlsClient.CheckExists(dirName));
}

[Fact(Skip = SkipMessage)]
public void TestIsDirectoryValidDirectoryE2E()
{
string dirName = $"/{_defaultFolderName}";
_adlsClient.CreateDirectory(dirName);
Assert.True(_fileSystem.IsDirectory(PathToFile(dirName)));
}

[Fact(Skip = SkipMessage)]
public void TestIsDirectoryFakeDirectoryE2E()
{
string fakeDirName = $"/fakeDir";
Assert.False(_fileSystem.IsDirectory(PathToFile(fakeDirName)));
}

[Fact(Skip = SkipMessage)]
public void TestIsDirectoryFileE2E()
{
string fileName = UploadFromString(ContentsText);
Assert.False(_fileSystem.IsDirectory(PathToFile(fileName)));
}

[Fact(Skip = SkipMessage)]
public void TestDeleteDirectoryE2E()
{
Expand Down
59 changes: 53 additions & 6 deletions lang/cs/Org.Apache.REEF.IO.Tests/TestHadoopFileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ namespace Org.Apache.REEF.IO.Tests
/// <see cref="HadoopFileSystem" />
public sealed class TestHadoopFileSystem
{
private const string SkipMessage = "These tests need to be run in an environment with HDFS installed."; // Use null to run tests
private HadoopFileSystem _fileSystem;

private Uri GetTempUri()
Expand All @@ -56,7 +57,7 @@ public TestHadoopFileSystem()
/// <summary>
/// Creates a temp file locally, uploads it to HDFS and downloads it again.
/// </summary>
[Fact(Skip = "These tests need to be run in an environment with HDFS installed.")]
[Fact(Skip = SkipMessage)]
public void TestCopyFromLocalAndBack()
{
var localFile = FileSystemTestUtilities.MakeLocalTempFile();
Expand All @@ -77,7 +78,7 @@ public void TestCopyFromLocalAndBack()
/// <summary>
/// Tests whether .Exists() works.
/// </summary>
[Fact(Skip = "These tests need to be run in an environment with HDFS installed.")]
[Fact(Skip = SkipMessage)]
public void TestExists()
{
var remoteUri = GetTempUri();
Expand All @@ -93,7 +94,7 @@ public void TestExists()
/// <summary>
/// Tests for .GetChildren().
/// </summary>
[Fact(Skip = "These tests need to be run in an environment with HDFS installed.")]
[Fact(Skip = SkipMessage)]
public void TestGetChildren()
{
// Make a directory
Expand Down Expand Up @@ -129,14 +130,14 @@ public void TestGetChildren()
_fileSystem.DeleteDirectory(remoteDirectory);
}

[Fact(Skip = "These tests need to be run in an environment with HDFS installed.")]
[Fact(Skip = SkipMessage)]
public void TestOpen()
{
// Open() is not supported by HadoopFileSystem. Use CopyToLocal and open the local file instead.
Assert.Throws<NotImplementedException>(() => _fileSystem.Open(GetTempUri()));
}

[Fact(Skip = "These tests need to be run in an environment with HDFS installed.")]
[Fact(Skip = SkipMessage)]
public void TestCreate()
{
// Create() is not supported by HadoopFileSystem. Create a local file and use CopyFromLocal instead.
Expand All @@ -163,10 +164,56 @@ public void CreateUriForPathWithPrefix()
Assert.Equal(new Uri(uriString), _fileSystem.CreateUriForPath(uriString));
}

/// <summary>
/// Tests whether .IsDirectory() works for valid directory.
/// </summary>
[Fact(Skip = SkipMessage)]
public void TestIsDirectoryValidDirectory()
{
// Create directory
var remoteDirUri = GetTempUri();
_fileSystem.CreateDirectory(remoteDirUri);

Assert.True(_fileSystem.IsDirectory(remoteDirUri));

// Clean up
_fileSystem.DeleteDirectory(remoteDirUri);
}

/// <summary>
/// Tests whether .IsDirectory() works for fake directory.
/// </summary>
[Fact(Skip = SkipMessage)]
public void TestIsDirectoryFakeDirectory()
{
// Create fake directory uri
var remoteFakeuri = GetTempUri();

Assert.False(_fileSystem.IsDirectory(remoteFakeuri));
}

/// <summary>
/// Tests whether .IsDirectory() works for file.
/// </summary>
[Fact(Skip = SkipMessage)]
public void TestIsDirectoryFile()
{
// Create file
var remoteFileUri = GetTempUri();
var localFile = FileSystemTestUtilities.MakeLocalTempFile();
_fileSystem.CopyFromLocal(localFile, remoteFileUri);

Assert.False(_fileSystem.IsDirectory(remoteFileUri));

// Clean up
_fileSystem.Delete(remoteFileUri);
File.Delete(localFile);
}

/// <summary>
/// This test is to make sure with the HadoopFileSystemConfiguration, HadoopFileSystem can be injected.
/// </summary>
[Fact(Skip = "These tests need to be run in an environment with HDFS installed.")]
[Fact(Skip = SkipMessage)]
public void TestHadoopFileSystemConfiguration()
{
var fileSystemTest = TangFactory.GetTang().NewInjector(HadoopFileSystemConfiguration.ConfigurationModule
Expand Down
23 changes: 23 additions & 0 deletions lang/cs/Org.Apache.REEF.IO.Tests/TestLocalFileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,29 @@ public void TestCopy()
Assert.False(fs.Exists(sourceUri));
}

[Fact]
public void TestIsDirectory()
{
var fs = GetFileSystem();
// Directory check
var directoryUri = new Uri(Path.Combine(Path.GetTempPath(), "dir"));
fs.CreateDirectory(directoryUri);
Assert.True(fs.IsDirectory(directoryUri), ".IsDirectory() failed on: " + directoryUri);

// File check
var fileUri = new Uri(directoryUri, "testfile");
MakeRemoteTestFile(fs, fileUri);
Assert.False(fs.IsDirectory(fileUri), ".IsDirectory() failed on: " + fileUri);

// Fake directory check
var fakeDirectoryUri = new Uri(Path.Combine(Path.GetTempPath(), "fakeDir"));
Assert.False(fs.IsDirectory(fakeDirectoryUri), ".IsDirectory() failed on: " + fakeDirectoryUri);

// Clean up
fs.Delete(fileUri);
fs.DeleteDirectory(directoryUri);
}

[Fact]
public void TestGetChildren()
{
Expand Down
23 changes: 21 additions & 2 deletions lang/cs/Org.Apache.REEF.IO/FileSystem/AzureBlob/AzureBlockBlobFileSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,32 @@ public void CreateDirectory(Uri directoryUri)
{
}

/// <summary>
/// Checks if uri is a directory uri.
/// </summary>
/// <param name="uri">uri of the directory/file</param>
/// <returns>true if uri is for a directory else false</returns>
public bool IsDirectory(Uri uri)
{
var path = uri.AbsolutePath.TrimStart(UrlPathSeparator);
var blobItems = _client.ListBlobsSegmented(
path,
useFlatListing: false,
BlobListingDetails.Metadata,
maxResults: 1,
continuationToken: null,
blobRequestOptions: null,
operationContext: null).Results;
return blobItems.OfType<CloudBlobDirectory>().Any();
}

/// <summary>
/// Recursively deletes blobs under a specified "directory URI."
/// If only the container is specified, the entire container is deleted.
/// </summary>
public void DeleteDirectory(Uri directoryUri)
{
var uriSplit = directoryUri.AbsolutePath.Split(new[] { "/" }, StringSplitOptions.RemoveEmptyEntries);
var uriSplit = directoryUri.AbsolutePath.Split(new[] { UrlPathSeparator }, StringSplitOptions.RemoveEmptyEntries);
if (!uriSplit.Any())
{
throw new StorageException(
Expand Down Expand Up @@ -197,7 +216,7 @@ public IEnumerable<Uri> GetChildren(Uri directoryUri)

do
{
BlobResultSegment listing = _client.ListBlobsSegmented(
BlobResultSegment listing = _client.ListDirectoryBlobsSegmented(
containerName,
relativeAddress,
useFlatListing: false,
Expand Down
Loading