Added Share Soft Delete#11260
Conversation
seanmcc-msft
requested review from
JoshLove-msft,
KrzysztofCwalina,
amnguye,
kasobol-msft,
pakrym and
tg-msft
as code owners
| // It takes some time for the Share to be deleted. | ||
| if (Mode != RecordedTestMode.Playback) | ||
| { | ||
| await Task.Delay(30000); |
There was a problem hiding this comment.
Would be better to wait on/poll (with exponential retry) some API for delete condition instead of hard-coded wait time. Might give more stability in live mode.
There was a problem hiding this comment.
Could we just keep calling Delete periodically until we stop getting ShareBeingDeleted or whatever the error code is? If you do keep this as-is, we've got a handy https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Common/tests/Shared/StorageTestBase.cs#L338 that encapsulates the mode check.
There was a problem hiding this comment.
Calling delete multiple tests doesn't work, delete doesn't return an error code if a share is being deleted. I'll switch to using the Delay method.
There was a problem hiding this comment.
I think we can change this before merging to master to calling Restore() in a loop and waiting for no error, for stability in the live tests.
|
|
||
| // Act | ||
| await TestHelper.AssertExpectedExceptionAsync<RequestFailedException>( | ||
| share.RestoreAsync(GetNewShareName(), "01D60F8BB59A4652"), |
There was a problem hiding this comment.
nit: can you introduce descriptive constant or variable for that string?
|
@tg-msft, could you look at this PR next? It's relatively small. |
tg-msft
left a comment
tg-msft
left a comment
There was a problem hiding this comment.
The only major question I have is about the name (also in ApiView). We called this Undelete for Blobs and we're using Restore here. Any reason we don't want Undelete for both?
| /// Gets a tenant to use for any tests related to blob or container soft delete. | ||
| /// </summary> | ||
| public static TenantConfiguration DefaultTargetSoftDeleteTenant => | ||
| GetTenant("TargetBlobAndContainerSoftDeleteTenant", s_configurations.Value.TargetSoftDeleteTenantName); |
There was a problem hiding this comment.
Is Soft Delete an account level setting that applies regardless of whether you're using Blobs/Files?
There was a problem hiding this comment.
No, there is Container Soft Delete and Blob Soft Delete for the Blobs service, and Share Soft Delete for Files. I'd like to just make 1 new test account with all soft delete features enabled.
The reason I want a separate account is that 1. it takes time to enable/disable these features on an account, and 2. if you list with the "Deleted" option, and soft delete has been enabled for weeks, the resulting list could be huge.
There was a problem hiding this comment.
Got it - I'm on board with the separate account. Just wonder if we should call the tenant name "TargetBlobsAndFilesSoftDeleteTenant" or just "TargetSoftDeleteTenant" since you need to enable it for both services? I don't feel very strongly here.
There was a problem hiding this comment.
I'll add additional documentation around what settings this tenant requres.
| ClientDiagnostics, | ||
| Pipeline, | ||
| Uri, | ||
| Version.ToVersionString(), |
There was a problem hiding this comment.
Does the operation just fail if you call this with an older service version? I can't imagine this causing any problems, but want to double check we don't need a version guard here.
There was a problem hiding this comment.
Yes it will fail, with a message from the service saying the version is incorrect. Peter decided we weren't doing version guarding in the SDK.
There was a problem hiding this comment.
I thought it was specifically no guards unless it could lead to data corruption or loss - which is why I like double checking for some of these new operations.
There was a problem hiding this comment.
Nothing will happen to your share if you call Restore with a service version that doesn't support it - you will just get a 400 class error.
| // It takes some time for the Share to be deleted. | ||
| if (Mode != RecordedTestMode.Playback) | ||
| { | ||
| await Task.Delay(30000); |
There was a problem hiding this comment.
Could we just keep calling Delete periodically until we stop getting ShareBeingDeleted or whatever the error code is? If you do keep this as-is, we've got a handy https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Common/tests/Shared/StorageTestBase.cs#L338 that encapsulates the mode check.
If we want to keep the APIs consistent, I'd prefer "Restore" over "Undelete". For the original naming of the APIs, I just copied the service. |
I'll attempt to call Delete in a loop until the error code changes and see what happens. |
Should we rename |
Ok, I'll leave the API name as-in for now, and we can circle back around once 73 is closer to the main Arch Board review. |
3 participants
No description provided.