[DO NOT MERGE] Feature/storage/hns soft delete #17054
Conversation
seanmcc-msft
requested review from
amishra-dev,
amnguye,
kasobol-msft,
KrzysztofCwalina and
tg-msft
as code owners
ghost
added
the
| @@ -4,7 +4,7 @@ | |||
| ## Configuration | |||
| ``` yaml | |||
| # Generate file storage | |||
| input-file: https://raw.githubusercontent.com/Azure/azure-rest-api-specs/storage-dataplane-preview/specification/storage/data-plane/Microsoft.StorageDataLake/stable/2020-02-10/DataLakeStorage.json | |||
| input-file: C:\Users\seanmcc\git\azure-rest-api-specs\specification\storage\data-plane\Microsoft.StorageDataLake\stable\2020-06-12\DataLakeStorage.json | |||
There was a problem hiding this comment.
I will revert this before merging.
| @@ -2,17 +2,17 @@ pushd $PSScriptRoot/Azure.Storage.Common/swagger/Generator/ | |||
| npm install | |||
| npm install -g autorest@beta | |||
|
|
|||
| cd $PSScriptRoot/Azure.Storage.Blobs/swagger/ | |||
| autorest --use=$PSScriptRoot/Azure.Storage.Common/swagger/Generator/ --verbose | |||
| #cd $PSScriptRoot/Azure.Storage.Blobs/swagger/ | |||
There was a problem hiding this comment.
I will revert this file before merging.
| @@ -761,6 +761,195 @@ public virtual DataLakeFileSystemClient GetFileSystemClient(string fileSystemNam | |||
| } | |||
| #endregion Delete File System | |||
|
|
|||
| #region Get Service Properties | |||
There was a problem hiding this comment.
I added DataLakeServiceClient.GetProperties() and .SetProperties() so customers can programmatically enable and disable HNS soft delete using the Data Lake SDK.
Note that DataLakeServiceProperties does not contain StaticWebsite. This is by design, I found during testing that HNS accounts do not support StaticWebsite.
| /// A <see cref="RequestFailedException"/> will be thrown if | ||
| /// a failure occurs. | ||
| /// </remarks> | ||
| public virtual Response<DataLakePathClient> RestorePath( |
There was a problem hiding this comment.
I talked to Kamil, and we chose to go with DataLakeFileSystemClient.RestorePath() for now, instead of .RestoreFile() and .RestoreDirectory.
Also note that this API is on the FileSystemClient. This is because you will be able to rename paths in the future when you restore them. I followed the precedent of BlobServiceClient.RestoreContainer() and ShareServiceClient.RestoreShare().
| @@ -2378,5 +2378,241 @@ private void SetNameFieldsIfNull() | |||
| return sasUri.ToUri(); | |||
| } | |||
| #endregion | |||
|
|
|||
| #region Get Deleted Paths | |||
There was a problem hiding this comment.
Note the limit amount of properties we are returning for the paths in GetDeletedPaths(). I was told by the FE team to try to stick with delete-related properties only.
There are probably a lot of properties we could pull from the swagger here - Azure/azure-rest-api-specs#11741
There was a problem hiding this comment.
- Note that I copied BlobContainerClient.GetBlobsHierarchical().
- GetDeletedPaths() doesn't currently support the delimiter parameter, but it will in the future.
- I didn't want to create to APIs for GetDeletedPaths(), I thought it was unnecessary.
| Assert.AreEqual(1, paths.Count); | ||
| Assert.AreEqual($"{directoryName}/{fileName}", paths[0].Path.Name); | ||
| } | ||
|
|
There was a problem hiding this comment.
TODO add unit test GetDeletedPathsAsync_PathSize()
| /// <summary> | ||
| /// The <see cref="GetProperties"/> operation gets the properties | ||
| /// of a storage account’s Data Lake service, including properties for | ||
| /// Storage Analytics and CORS (Cross-Origin Resource Sharing) rules. |
There was a problem hiding this comment.
should this mention retention settings? (and Set as well)
| public bool Enabled { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Indicates the number of days that metrics or logging or soft-deleted data should be retained. All data older than this value will be deleted. |
There was a problem hiding this comment.
maybe we shouldn't mention what this setting can affect. I.e. don't mention logging soft-deleted etc. this can be derived from where property of that type is nested.
I made a mistake and read this comment first before realizing that property of this type exists in many places and made me wonder why I'd set soft delete retention in metrics configuration.
Somebody who's full text searching codebase/docs may fall in such trap.
Besides, if this class is used in more places we'd most likely forget to update this doc.
There was a problem hiding this comment.
This was copied almost exactly from BlobRetentionPolicy.
| /// <summary> | ||
| /// The retention policy which determines how long the associated data should persist. | ||
| /// </summary> | ||
| public DataLakeRetentionPolicy DeleteRetentionPolicy { get; set; } |
There was a problem hiding this comment.
In other places it's easy to figure out what this property mean as they're nested inside some other named part of configuration.
Assuming that this controls how long files/directories exist I think we should be more verbose in naming and doc.
I.e. "The retention policy which determines how long the deleted files and directories should persist."
Maybe this should be called DeletedPathsRetentionPolicy ?
There was a problem hiding this comment.
This name is pattern-matched from BlobRetentionPolicy.
seanmcc-msft
changed the title
|
Hi @seanmcc-msft. There hasn't been recent engagement on this PR. Would you please be so kind as to let us know if this is still an active work stream or if the PR should be closed out? |
jsquire
added
the
no-recent-activity
|
@jsquire please leave this open. |
jsquire
removed
the
no-recent-activity
No description provided.