[Update and enable storage command]: reset command for storage account #200
Conversation
ams/ResetCommand.cs
Outdated
| int resetAssetCount = 0; | ||
| foreach (var asset in assetList) | ||
| } | ||
| protected async Task MigrateTaskAsync(BlobContainerClient container, int resetAssetCount, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Two issues on this function:
(1) param int resetAssetCount should be changed to "ref int resetAssetCount", because this value is updated inside the function, and it should be reported back to the caller.
(2) protected can be changed to private, there is no derived class from ResetCommand.
ams/ResetCommand.cs
Outdated
| } | ||
| _logger.LogDebug("{resetAssetCount} out of {totalAssetCount} assets has been reset.", resetAssetCount, assetList.Count); | ||
| } | ||
| else |
There was a problem hiding this comment.
The code here should be moved to the place where it is not an AMS account, nor Storage Account. A good place is to move to after line 39, when it is not a storage account, throw the exception.
ams/ResetCommand.cs
Outdated
| } | ||
| catch (Exception) | ||
| else if (account != null) |
There was a problem hiding this comment.
If IsAMSAccountAsync return true, would the account is always not null, this check seems not right.
The similar code pattern applies to all other classes.
ams/BaseMigrator.cs
Outdated
| { | ||
| return (false, account); | ||
| } | ||
| return account ==null?(false, account):(true, account); |
There was a problem hiding this comment.
In what case, when this exception is throw, but account is still not null?
ams/BaseMigrator.cs
Outdated
| try | ||
| { | ||
| account = await GetMediaAccountAsync(accountName, cancellationToken); | ||
| return (true, account); | ||
| } | ||
| catch (RequestFailedException ex) | ||
| { | ||
| if (ex.ErrorCode != null && ex.ErrorCode.Equals("ResourceNotFound")) | ||
| { | ||
| return (false, account); | ||
| } | ||
| return account ==null?(false, account):(true, account); |
There was a problem hiding this comment.
This function can be changed as below:
protected async Task<(bool, MediaServicesAccountResource?)> IsAMSAccountAsync(string accountName, CancellationToken cancellationToken)
{
MediaServicesAccountResource? amsAccount = null;
try
{
amsAccount = await _resourceProvider.GetMediaAccountAsync(accountName, cancellationToken);
}
catch (Exception ex)
{
if (ex is OutOfMemoryException) throw; // It is a fatal error.
// For any other exception, swallow the exception, treat it as not-AMS account,
// The caller then has a chance to treat it as storage account and try it again,
// if it is still failed, the caller will throw exception appropriately.
}
return (amsAccount != null, amsAccount);
}
Once this function returns true, the AmsAccount must be available, if it is false, the caller should do further check if it is a storage account, otherwise, report error or throw exception in the caller side.
work item:#192