-
Notifications
You must be signed in to change notification settings - Fork 19
[Package Renames 4] Refactor download overrides #768
Conversation
18ac316
to
028849b
Compare
6bbc513
to
010687c
Compare
010687c
to
432dd7c
Compare
return await GetTransferChangesAsync( | ||
downloads, | ||
outgoingTransfers); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InitializeDownloadTransfers
and UpdateDownloadTransfers
are very similar right now, but they will diverge in a subsequent change.
Task<DownloadTransferResult> GetUpdatedTransferChangesAsync( | ||
DownloadData downloads, | ||
SortedDictionary<string, long> downloadChanges, | ||
SortedDictionary<string, SortedSet<string>> oldTransfers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentionally not using IReadOnlyX
interfaces as this method needs to check the .Comparer
property.
_logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
} | ||
|
||
public async Task<DownloadTransferResult> GetTransferChangesAsync(DownloadData downloads) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about the name InitializeTransferChangesAsync
? Seems like this should only be called in a case like Db2AzureSearch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed this to InitializeDownloadTransfers
and renamed the other method to UpdateDownloadTransfers
src/NuGet.Services.AzureSearch/Auxiliary2AzureSearch/UpdateDownloadsCommand.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -195,6 +201,12 @@ private async Task WriteVerifiedPackagesDataAsync(HashSet<string> verifiedPackag | |||
_logger.LogInformation("Done uploading the initial verified packages data file."); | |||
} | |||
|
|||
private async Task WritePopularityTransfersDataAsync(SortedDictionary<string, SortedSet<string>> popularityTransfers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest moving this to a common place and call this from auxiliary2azuresearch
TODO above... the implementation can be TODO, one less thing to worry about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this comment as this code path is separate from auxiliary2azuresearch
.
In any case, I removed this method since having an empty method is weird though
src/NuGet.Services.AzureSearch/Db2AzureSearch/NewPackageRegistrationProducer.cs
Outdated
Show resolved
Hide resolved
Update tests Update comments Update comments Update Db2AzureSearchCommandFacts Update new package registration producer facts Add test Add tests for download transferrer Rename method Fix test Don't save popularity transfers for now Work Fix Fix Clean
653afdb
to
49cac2c
Compare
|
||
public SortedDictionary<string, long> InitializeDownloadTransfers( | ||
DownloadData downloads, | ||
SortedDictionary<string, SortedSet<string>> outgoingTransfers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing a guard. I added this in the subsequent PR: https://github.com/NuGet/NuGet.Services.Metadata/pull/769/files
(Porting the changes from the next PR back into this one is a little work intensive, hence why I punted it to the next PR)
DownloadData downloads, | ||
SortedDictionary<string, long> downloadChanges, | ||
SortedDictionary<string, SortedSet<string>> oldTransfers, | ||
SortedDictionary<string, SortedSet<string>> newTransfers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing a guard. I added this in the subsequent PR: https://github.com/NuGet/NuGet.Services.Metadata/pull/769/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better. Ship it.
This change introduces a new type,
IDownloadTransferrer
, which determines what downloads should be changed to reflect the latest download overrides. In the future, this type will also apply popularity transfers.Previous changes: #765, #766, and #767.
Part of NuGet/NuGetGallery#7898
Background
Originally, auxiliary2azuresearch updated downloads with these steps:
Later we added download overrides. The steps became:
Now we want to add popularity transfers. These popularity transfers are affected by download changes and can be overriden by download overrides. The new steps will be: