Bugfix for PostProcess Stats job #10282
Conversation
| @@ -111,9 +111,12 @@ public override async Task<IEnumerable<StorageListItem>> ListAsync(bool getMetad | |||
| return await _primaryStorage.ListAsync(getMetadata, cancellationToken); | |||
| } | |||
|
|
|||
| public override async Task<IEnumerable<StorageListItem>> ListTopLevelAsync(bool getMetadata, CancellationToken cancellationToken) => | |||
| await ListTopLevelAsync(getMetadata, cancellationToken); | |||
There was a problem hiding this comment.
Does it call itself? It will fail with stack overflow exception.
Should it be _primaryStorage.ListTopLevelAsync?
There was a problem hiding this comment.
Thanks for catching that, fixing now
| @@ -57,6 +57,9 @@ public override Task<IEnumerable<StorageListItem>> ListAsync(bool getMetadata, C | |||
| return Task.FromResult<IEnumerable<StorageListItem>>(List(getMetadata)); | |||
| } | |||
|
|
|||
| public override Task<IEnumerable<StorageListItem>> ListTopLevelAsync(bool getMetadata, CancellationToken cancellationToken) => | |||
| ListTopLevelAsync(getMetadata, cancellationToken); | |||
There was a problem hiding this comment.
Similar concern here. It should not call itself, but actually list files like List method does above, but without recursing into subdirectories. However, this seems to unnecessary broaden the scope of this change. Consider throwing NotImplementedException here and filing a work item to implement it.
There was a problem hiding this comment.
Fixed, work item filed here
| @@ -19,7 +20,13 @@ public interface IStorage | |||
| Uri BaseAddress { get; } | |||
| Uri ResolveUri(string relativeUri); | |||
| IEnumerable<StorageListItem> List(bool getMetadata); | |||
|
|
|||
| //Lists all children of the storage(including the ones contained in subdirectories). | |||
There was a problem hiding this comment.
Please, use the documentation comment syntax
/// <summary>
/// ...
/// </summary>
| @@ -209,6 +210,12 @@ protected string GetName(Uri uri) | |||
| { | |||
| name = name.Substring(0, name.IndexOf("#")); | |||
| } | |||
|
|
|||
| if (name.Contains("?")) | |||
There was a problem hiding this comment.
I believe the query (?) comes before the fragment (#), so it would be better to do this check first. That way you don't allocate a substring twice if both exist.
[edit:] With reordering these, you can also make the second check an else if because if you find a ? you no longer need to check for #
Alternatively, you could do a single pass with IndexOfAny, which would also save a second iteration over the string.
|
@Lanaparezanin |
Bug description: existing method was returning an extra entry for the path itself, which caused the job to break as it couldn't process the extra entry. This was due to changes in the storage account and the new SDK.
The new method handles the extra entry without affecting existing jobs. It ensures that only the relevant blobs are fetched, avoiding the extra entry that caused the job to break.
Delimiter details: the new SDK required a trailing slash in the path to function correctly. Without this trailing slash, the method would return an extra entry for the path itself.
Other changes are an update to ResolveUri, the existing method wasn't sending proper information of the path and this one is. This bug might be related to other jobs as well and fixing it might break things. I filed it here: https://github.com/orgs/NuGet/projects/21/views/1?filterQuery=milestone%3A%22Sprint+2024-12%22+assignee%3A%40me&pane=issue&itemId=89578571&issue=NuGet%7CEngineering%7C5723
Addresses https://github.com/orgs/NuGet/projects/21/views/1?filterQuery=milestone%3A%22Sprint+2024-11%22+assignee%3A%40me&pane=issue&itemId=64259916&issue=NuGet%7CEngineering%7C5445