[Prefix] Display Packages' Verified Status on Search Service #226
Conversation
loic-sharma
requested review from
joelverhagen,
shishirx34,
ryuyu and
scottbommarito
| @@ -340,6 +342,7 @@ private static void WriteDataV2(JsonWriter jsonWriter, NuGetIndexSearcher search | |||
| WriteDocumentValue(jsonWriter, "ReleaseNotes", document, "ReleaseNotes"); | |||
| WriteDocumentValue(jsonWriter, "ProjectUrl", document, "ProjectUrl"); | |||
| WriteDocumentValue(jsonWriter, "IconUrl", document, "IconUrl"); | |||
| WriteProperty(jsonWriter, "IsVerified", isVerified); | |||
There was a problem hiding this comment.
For v2 this needs to be in PackageRegistration object, WriteRegistrationV2 above. See the OneNote spec for the return result structure. Also the key should be "Verified" for V2 search result on the registration object.
| @@ -0,0 +1,59 @@ | |||
| using System; | |||
|
|
||
| namespace NuGet.Indexing | ||
| { | ||
| public static class VerifiedPackages |
There was a problem hiding this comment.
Class can be internal. NuGet.Indexing has internals visible to NuGet.IndexingTests.
|
|
||
| namespace NuGet.IndexingTests | ||
| { | ||
| public class VerifiedPackagesParsingTests |
There was a problem hiding this comment.
Tests should mirror source classes. I would rename to VerifiedPackagesTests.
Also, consider making VerifiedPackages.Parse private and testing through the Load API which is used directly by callers.
There was a problem hiding this comment.
Testing the Load method would require mocks that would complicate the test for little gain. Since the Load method is only a convenience method, I think testing the Parse method directly is fine. Thoughts?
There was a problem hiding this comment.
No strong opinion here. Generally, I try to test the entry point for complete coverage.
I thought it'd just require a simple mock to have the loader return a reader... but if it's more complex, wouldn't worry about it.
| [InlineData("['test']", new string[] { "test" })] | ||
| [InlineData("['test', 'test']", new string[] { "test" })] | ||
| [InlineData("['test', 'test123']", new string[] { "test", "test123" })] | ||
| public void ParsesProperInput(string input, string[] expected) |
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using Xunit; |
There was a problem hiding this comment.
nit: sort usings, System first
|
|
||
| while (packageId != null) | ||
| { | ||
| result.Add(String.Intern(packageId)); |
There was a problem hiding this comment.
Assuming String.Intern is intended to optimize lookup performance? Would the string for lookup need to be interned as well, and would the optimization work on a case insensitive collection?
Would measure if this code actually improves perf (i.e., with simple console app), and keep / add comment if it does.
There was a problem hiding this comment.
Interning strings is a pattern used throughout the search service. String.Intern is to reduce memory pressure, not to improve performance. Each reload allocates a bunch of strings (things like package IDs, versions, etc..) that already exist in memory from the previous index. By interning the string, we can reuse the memory that was allocated the first time that string was encountered. The memory allocated during the index reload will then be collected on the next garbage collection. If we didn't intern the strings, the strings would be allocated on each reload and would need to be freed on the subsequent reload. Since the time between reloads is in the order of minutes, the strings would likely get promoted to the next generation of memory. Garbage collection is cheaper when memory is only used briefly.
There was a problem hiding this comment.
Is this documented in code, so we don't accidentally remove it?
| @@ -6,6 +6,10 @@ | |||
| using NuGet.Services.BasicSearchTests.Models; | |||
| using NuGet.Services.BasicSearchTests.TestSupport; | |||
| using Xunit; | |||
| using System.IO; | |||
| using Newtonsoft.Json; | |||
| using System; | |||
| @@ -206,7 +207,7 @@ protected override NuGetIndexSearcher CreateSearcher(IndexReader reader) | |||
| } | |||
| catch (Exception e) | |||
| { | |||
| _logger.LogError("NuGetSearcherManager.CreateSearcher: Error loading auxiliary data.", e); | |||
| _logger.LogError("NuGetSearcherManager.CreateSearcher - Exception thrown while loading auxiliary data: {Exception}", e); | |||
There was a problem hiding this comment.
if verifiedPackages.json is not found would this exception not reload any aux data? I am worried what would happen if we deployed search service before the job is deployed.
There was a problem hiding this comment.
If verifiedPackages.json is not found, the search service will fail to launch and all responses will return UNINITIALIZED. I believe this is the correct pattern to use - if there are issues, the search service should fail so that we are alerted quickly. We should deploy this change only when the auxiliary data is ready to be consumed.
There was a problem hiding this comment.
Please update the deployment task to make sure that the order of deployment is important during this deployment. NuGet/NuGetGallery#4591
Fixes NuGet/NuGetGallery#4382