[REEF-952] IFileSystem for Azure BlockBlobs #645
Conversation
| var uriSplit = directoryUri.AbsolutePath.Split(new[] {"/"}, StringSplitOptions.RemoveEmptyEntries); | ||
| if (!uriSplit.Any()) | ||
| { | ||
| throw new ArgumentOutOfRangeException(@"URI must contain at least the container"); |
There was a problem hiding this comment.
This exception type is artifact of implementation detail. Use exception type that reflects the semantics.
This addressed the issue by * Adding Azure Storage dependency to REEF.IO. * Implementing IFileSystem for Block Blobs. * Implementing and testing on "Fakes" and actual blob storage. * Creating proxy classes for "Fakes." JIRA: [REEF-952](https://issues.apache.org/jira/browse/REEF-952)
|
|
||
| public async Task<string> StartCopyFromBlobAsync(Uri source) | ||
| { | ||
| return await _blob.StartCopyFromBlobAsync(source); |
There was a problem hiding this comment.
Consider using RemoveSynchronizationContextAwaiter at async entry points.
|
Changes LGTM in general. Left a few comments. |
|
@anupam128 Ready for a second review. Thanks! |
| <HintPath>$(PackagesDir)\NSubstitute.1.8.2.0\lib\net45\NSubstitute.dll</HintPath> | ||
| <Private>True</Private> | ||
| </Reference> | ||
| <Reference Include="Microsoft.WindowsAzure.Storage, Version=4.3.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL"> |
There was a problem hiding this comment.
This version seems outdated. There is a version [6.1.0])https://www.nuget.org/packages/WindowsAzure.Storage/) available. Why are we sticking with this one?
There was a problem hiding this comment.
We already have a reference to WindowsAzure.Storage 4.3.0 in Org.Apache.REEF.Tests. Should I upgrade that as well? Or should I just stick with this version?
|
@afchung I've done a pass. |
|
@markusweimer Addressed your comments, please have another look. Thanks! |
|
+1 |
| { | ||
| _configuration = TangFactory.GetTang().NewConfigurationBuilder() | ||
| .BindImplementation(GenericType<IFileSystem>.Class, GenericType<AzureBlockBlobFileSystem>.Class) | ||
| .BindImplementation(typeof(IAzureBlobRetryPolicy), retryPolicy.GetType()) |
There was a problem hiding this comment.
If what you need is type only, you can just pass it as a named parameter of string, then get type from it. In the current way, you create an object of AzureBlobRetryPolicy but not really use it except type.
There was a problem hiding this comment.
Talked with @jwang98052 offline. Provided that the IAzureBlobRetryPolicy is a small class, I will leave as is.
|
+1 on merging. Can someone confirm to have run tests on this? |
|
Sorry, pushed to wrong branch, will revert. |
|
Reverted. |
|
@afchung and I just spoke, he tested this code on Azure. I will merge it. |
|
This doesn't build for me. @afchung Can you have a look? The build fails due to missing metadata files for DLLs. Likely abuild order issue. |
|
@markusweimer I was able to build it on a new clone. What were your build errors? |
|
Seems like our builds are a bit unstable... for another PR I had to build 3 times before actually succeeding. |
|
I will try on the command line and see whether that works. If it does, I will merge. |
This addressed the issue by * Adding Azure Storage dependency to REEF.IO. * Implementing IFileSystem for Block Blobs. * Implementing and testing on "Fakes" and actual blob storage. * Creating proxy classes for "Fakes." JIRA: [REEF-952](https://issues.apache.org/jira/browse/REEF-952) Pull Request: This closes apache#645
This addressed the issue by
JIRA:
REEF-952