Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tilovell 908 987 curatedfeed perf and package uniqueness #1002

Conversation

TimLovellSmith
Copy link
Member

Addresses #908 and #987. Curated Feeds are now run from the lucene Index, which has been modified to include a field stating which curated feed a package is included in. This greatly improves curated feed search performance. This PR also includes the change to make the CuratedPackages table properly uniquely indexable by pair (CuratedFeedKey, PackageRegistrationKey) so that we disallow adding dupes.

Tim Lovell-Smith added 7 commits April 1, 2013 23:57
… package searches by filtering in terms of PackageRegistrations, not Packages, and thereby optimizing the SQL queries. Also adding a webpage for searching curated feeds, for debugging purposes only since nothing links to it.
…ratedFeedService abstraction for mocking. That service doesn't feel so bad, but the test does feel a bit contrived.
…d keys in the Lucene Index and using that for filtering the search results instead of hitting the database and merging lists.
…. This will move over to a SQL script in NuGetOperations.
public override void Up()
{
// ADD uniqueness constraint - as an Index, since it seems reasonable to look up curated package entries by their feed + registration
CreateIndex("CuratedPackages", new[] { "CuratedFeedKey", "PackageRegistrationKey" }, unique: true, name: "IX_CuratedFeed_PackageRegistration");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a "Contract" migration. Could you identify it as such? (Contract_CuratedFeedPackageUniqueness was my thought for a naming convention). And maybe pull it out into it's own change to be applied after we've deployed the code? It's definitely a good optimization but not technically required for the change to work, as far as I can tell.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not a contract in the 'breaking' sense, only in the 'has risk of failing sense'. Will pull into separate PR.

@jeffhandley
Copy link
Member

Only thing missing is the big fix for preventing duplicate records in the curated feed. Since the unique constraint is being added, now we'll get exceptions.

@jeffhandley jeffhandley reopened this Apr 10, 2013
@TimLovellSmith
Copy link
Member Author

I'll close this again as 1022 is the one I'm going to make the fix in, have noted your comment there also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants