-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Added DataLake SDK #8235
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.
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
Added DataLake SDK #8235
Conversation
|
/azp run net - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run net - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run net - storage - ci |
|
/azp run net - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2045d50 to
7e36037
Compare
sdk/storage/Azure.Storage.Common/tests/Shared/TestConfigurations.cs
Outdated
Show resolved
Hide resolved
sdk/storage/Azure.Storage.Common/tests/Shared/TestConfigurations.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This references Files too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, but oddly when I remove this project reference, I'm no longer able to reference System.Text.Json in the DataLake package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried copying https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Files/src/Azure.Storage.Files.csproj#L22 verbatim? If you install via NuGet it'll add a version which our build system will complain about (to ensure Files doesn't use one version and DataLake another). If that doesn't fix it easily, it's fine to check in like this but please file a bug so it's dealt with before we release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one can't be skipped. Every ClientOptions needs to have a ServiceVersion enum and take it in the .ctor following the pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? DataLakeClientOptions extends BlobClientOptions, so it has the ServiceVersion enum and exposes its parent's constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess maybe deriving from BlobClientOptions is more of the problem here. I'm not sure we should do that, but that's a GREAT thing to discuss during the API review on Thursday. Let's go in like this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I derived from BlobClientOptions because I don't see a reason why DataLakeClientOptions shouldn't be identical to BlobClientOptions other than the name of the class, and the namespace. Why write extra code for the same functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel sufficient because Dfs in my account name would trigger it. Could we write a DataLakeUriBuilder that takes this apart a little safer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea.
The overall problem I'm trying to solved with the Uris is that we need the DFS Uri, the Blob Uri, and want to return the original Uri the customer provided when they call DataLakeServiceClient.Uri.
Do you think we should have a DataLakeUriBuilder and a BlobUriBuilder in the DataLake namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just add a DataLakeUriBuilder and have ToDataLakeUri and ToBlobUri methods on it that puts the prefix in the right location.
sdk/storage/Azure.Storage.Files.DataLake/src/DataLakeServiceClient.cs
Outdated
Show resolved
Hide resolved
29bf217 to
6048b90
Compare
sdk/storage/Azure.Storage.Files.DataLake/src/DataLakeServiceClient.cs
Outdated
Show resolved
Hide resolved
sdk/storage/Azure.Storage.Files.DataLake/src/Models/PathAccessControl.cs
Outdated
Show resolved
Hide resolved
6048b90 to
d4df143
Compare
d4df143 to
d137727
Compare
No description provided.