Updating stats jobs to use new Azure SDK #10323
Conversation
src/Stats.AzureCdnLogs.Common/AzureHelpers/AzureBlobLeaseManager.cs
Outdated
Show resolved
Hide resolved
src/Stats.AzureCdnLogs.Common/AzureHelpers/AzureBlobLeaseManager.cs
Outdated
Show resolved
Hide resolved
| //start a task that will renew the lease until the token is cancelled or the Release methods was invoked | ||
| var renewStatusTask = new Task( (lockresult) => | ||
| var leaseResult = await _blobLeaseService.TryAcquireAsync(blob.Name, TimeSpan.FromSeconds(MaxRenewPeriodInSeconds), CancellationToken.None); | ||
| if (!leaseResult.IsSuccess) |
There was a problem hiding this comment.
nit: Please invert the order. Please check the success first then it's easier to read.
There was a problem hiding this comment.
Check the success before what?
| /// <returns>An <see cref="AzureBlobLockResult"/> indicating the result of the lease acquisition. | ||
| /// If the lease is successfully acquired, the result will contain the lease ID and a cancellation token | ||
| /// source that can be used to stop the lease renewal task.</returns> | ||
| public async Task<AzureBlobLockResult> AcquireLease(BlobClient blob) |
There was a problem hiding this comment.
is this used outside of stats job world? Can we rename this to AcquireLeaseAsync without breaking anything? If not, ignore this comment.
There was a problem hiding this comment.
It isn't, seems like this is the only reference:
src/Stats.AzureCdnLogs.Common/AzureHelpers/AzureBlobLeaseManager.cs
Outdated
Show resolved
Hide resolved
| return new AsyncOperationResult(true, null); | ||
| } | ||
|
|
||
| await resultStream.FlushAsync(); |
There was a problem hiding this comment.
It looks like the previous implementation had a second explicit check for blob existence before flushing/commit the data so that if another process wrote out the file while this one was processing, the action was cancelled. New one, later one wins. Do we know if this slight change has any tangible impact on what we copy?
| cancellationToken: token); | ||
| bool deleteResult = await sourceBlob.DeleteIfExistsAsync(DeleteSnapshotsOption.IncludeSnapshots, | ||
| conditions: blobRequestConditions, | ||
| cancellationToken: CancellationToken.None); |
There was a problem hiding this comment.
We should probably pass through the cancellation token passed in here (unless there is a specific reason we are passing through None).
|
Nit: Using more explicit type makes code more readable. |
| else throw new ArgumentException(nameof(containerName)); | ||
| } | ||
|
|
||
| _blobLeaseService = new BlobLeaseService(blobServiceClient, containerName, basePath); |
There was a problem hiding this comment.
This change introduces a potential issue: AzureBlobLeaseManager can be constructed with a BlobServiceClient configured for a certain storage account with a certain container and base path, and then AcquireLease can be called passing a blob that might reside in a different storage account, or different container or not use the base path passed into AzureBlobLeaseManager constructor.
There was a problem hiding this comment.
All 3 added arguments (blobServiceClient, containerName, basePath) seem to exist only to be passed into a BlobLeaseService constructor. I suggest to change AzureBlobLeaseManager constructor to accept a IBlobLeaseService object instead and update the job setup accordingly.
There was a problem hiding this comment.
Still, we need to sort out the blob validation. One way of doing that could be to add a TryAcquireAsync overload in BlobLeaseService that would accept a blob URL instead of a resourceName, validate its values against the object configuration then infer resourceName from URL and call the original implementation. Then this new method (passing in full blob URL) can be used in AzureBlobLeaseManager.AcquireLease.
| @@ -49,7 +48,7 @@ public async Task<BlobLeaseResult> TryAcquireAsync(string resourceName, TimeSpan | |||
|
|
|||
| try | |||
| { | |||
| return await TryAcquireAsync(blob, leaseTime, cancellationToken); | |||
| return await TryAcquireAsync(blob, leaseTime, cancellationToken); ; | |||
| } | |||
| catch (RequestFailedException ex) when (ex.Status == (int)HttpStatusCode.NotFound) | |||
There was a problem hiding this comment.
We need a way to configure the object not to try to create blobs when they were not found. I suggest adding a boolean flag to BlobLeaseService's (defaulting to true for backwards compatibility) constructor to indicate if it is allowed to create blobs when they are missing and check it here. Then make sure that for this job it is not allowed to create blobs.
|
|
||
| int sleepBeforeRenewInSeconds = MaxRenewPeriodInSeconds - OverlapRenewPeriodInSeconds < 0 ? MaxRenewPeriodInSeconds : MaxRenewPeriodInSeconds - OverlapRenewPeriodInSeconds; | ||
| if (!blobLockResult.BlobOperationToken.IsCancellationRequested) |
There was a problem hiding this comment.
I wonder why we had this check in the original code. Technically, it is checking for a different thing than the while loop below, but I think the check in while loop would cover all cases this check would test. Any ideas?
There was a problem hiding this comment.
Starting a task might be an expensive operation, so it's beneficial to check before entering the task to improve performance.
There was a problem hiding this comment.
There is no starting tasks between line 66 and 68. They literally are run one after another.
src/Stats.AzureCdnLogs.Common/Collect/AzureStatsLogDestination.cs
Outdated
Show resolved
Hide resolved
| blobLockResult.LeaseId); | ||
| } | ||
| catch (StorageException exception) | ||
| await Task.Delay(TimeSpan.FromSeconds(MaxRenewPeriodInSeconds - OverlapRenewPeriodInSeconds), lockResult.BlobOperationToken.Token); |
There was a problem hiding this comment.
Do not pass cancellation token into Task.Delay. It will throw if cancelled while waiting (unlike Thread.Sleep in the original code).
I updated the stats jobs (
Stats.CollectAzureChinaCDNLogsandStats.CollectAzureChinaCDNLogs) to use the new Azure Storage SDK.This is part of the work for stats jobs migration: https://github.com/orgs/NuGet/projects/21/views/1?filterQuery=milestone%3A%22Sprint+2025-01%22+assignee%3A%40me&pane=issue&itemId=64259916&issue=NuGet%7CEngineering%7C5445