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/Collect/AzureStatsLogDestination.cs
Outdated
Show resolved
Hide resolved
|
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.
|
|
||
| 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.
There was a problem hiding this comment.
I looked into it and I really can't figure out why we used it. It seems redundant.
src/Stats.AzureCdnLogs.Common/AzureHelpers/AzureBlobLeaseManager.cs
Outdated
Show resolved
Hide resolved
src/Stats.AzureCdnLogs.Common/Collect/AzureStatsLogDestination.cs
Outdated
Show resolved
Hide resolved
src/Stats.AzureCdnLogs.Common/AzureHelpers/AzureBlobLeaseManager.cs
Outdated
Show resolved
Hide resolved
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