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

search: paginated repository resolution (take 2) #27359

Merged
merged 9 commits into from Nov 11, 2021

Conversation

tsenart
Copy link
Contributor

@tsenart tsenart commented Nov 10, 2021

This PR is the last of series to introduce paginated repo resolution in search.

  • We remove backend.ListSearchable and its use in repository resolution because global searches on Sourcegraph.com no longer depend on it. This in turn simplifies the code in preparation for cursor based pagination.
  • In order to be able to remove the blocking resolveRepositories in doResults, repository search had to be migrated to the new Job interface. This is implemented by run.RepoSearch.
  • We migrate all existing Job implementations to use searchrepos.Resolver.Paginate.
  • Alerts for missing repo revs and no resolved repos errors are now handled by searchResolver.errorToAlert.

New stuff in take 2

  • We optimize run.RepoSearch to leverage the database for regex matching on repo name, rather than brute-forcing it through all paginated repos. This should address the massive extra DB load we saw in production yesterday.
  • After thinking more about and talking with @stefanhengl, we change the semantics of repository count stats to mean any repositories with matches, rather than "searched" or "searchable". Searched repositories and searchable repositories aren't as useful as the number of repositories that have matches.

Part of #26995

@cla-bot cla-bot bot added the cla-signed label Nov 10, 2021
@tsenart tsenart changed the title Backend integration/ts/paginated repo resolution search: paginated repository resolution (take 2) Nov 10, 2021
@tsenart tsenart force-pushed the backend-integration/ts/paginated-repo-resolution branch 2 times, most recently from b42794d to 2d6e36f Compare November 10, 2021 14:03
@tsenart tsenart requested review from a team November 10, 2021 14:06
@tsenart tsenart marked this pull request as ready for review November 10, 2021 14:06
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Nov 10, 2021

Notifying subscribers in CODENOTIFY files for diff db5d3f0...a815d2f.

Notify File(s)
@beyang internal/search/commit/commit_new.go
internal/search/repos/repos.go
internal/search/repos/repos_test.go
internal/search/run/aggregator.go
internal/search/run/repository.go
internal/search/run/repository_test.go
internal/search/run/run.go
internal/search/streaming/progress.go
internal/search/symbol/symbol.go
internal/search/types.go
internal/search/unindexed/structural.go
internal/search/unindexed/unindexed.go
@camdencheek cmd/frontend/internal/search/search.go
internal/search/commit/commit_new.go
internal/search/repos/repos.go
internal/search/repos/repos_test.go
internal/search/run/aggregator.go
internal/search/run/repository.go
internal/search/run/repository_test.go
internal/search/run/run.go
internal/search/streaming/progress.go
internal/search/symbol/symbol.go
internal/search/types.go
internal/search/unindexed/structural.go
internal/search/unindexed/unindexed.go
@eseliger internal/database/repos.go
internal/database/repos_db_test.go
internal/database/repos_test.go
@keegancsmith cmd/frontend/graphqlbackend/search.go
cmd/frontend/graphqlbackend/search_alert.go
cmd/frontend/graphqlbackend/search_alert_test.go
cmd/frontend/graphqlbackend/search_results.go
cmd/frontend/graphqlbackend/search_results_test.go
cmd/frontend/graphqlbackend/search_suggestions.go
cmd/frontend/internal/search/search.go
internal/search/commit/commit_new.go
internal/search/repos/repos.go
internal/search/repos/repos_test.go
internal/search/run/aggregator.go
internal/search/run/repository.go
internal/search/run/repository_test.go
internal/search/run/run.go
internal/search/streaming/progress.go
internal/search/symbol/symbol.go
internal/search/types.go
internal/search/unindexed/structural.go
internal/search/unindexed/unindexed.go
@rvantonder cmd/frontend/graphqlbackend/search.go
cmd/frontend/graphqlbackend/search_alert.go
cmd/frontend/graphqlbackend/search_alert_test.go
cmd/frontend/graphqlbackend/search_results.go
cmd/frontend/graphqlbackend/search_results_test.go
cmd/frontend/graphqlbackend/search_suggestions.go
@unknwon dev/gqltest/.tool-versions
dev/gqltest/search_test.go
internal/database/repos.go
internal/database/repos_db_test.go
internal/database/repos_test.go

@tsenart tsenart force-pushed the backend-integration/ts/paginated-repo-resolution branch 3 times, most recently from ef43625 to aa2a96b Compare November 10, 2021 17:07
Copy link
Contributor

@rvantonder rvantonder left a comment

Choose a reason for hiding this comment

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

Approving to unblock since I give a lot of weight to integration tests and they're green :-) I would check the behavior of the case-insensitive thing I commented on.

GLHF.

internal/search/run/repository.go Outdated Show resolved Hide resolved
internal/search/run/repository.go Show resolved Hide resolved
Copy link
Member

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Changes to the files that I subscribed LGTM.

This PR is the last of series to introduce paginated repo resolution in
search.

We remove `backend.ListSearchable` and its use in repository resolution
because global searches on Sourcegraph.com no longer depend on it. This
in turn simplifies the code in preparation for cursor based pagination.

Part of #26995
This commit optimizes the RepoSearch job by leveraging the database to
find repos based on the text pattern. Yesterday's production incident
was caused by excessive database requests due to our previous brute-force
approach of loading all repos and then filtering them in the frontend.
This commit makes our repo stats semantics (in the UI and GraphQL API)
mean repositories with matches rather than searched or searchable.
@tsenart tsenart force-pushed the backend-integration/ts/paginated-repo-resolution branch from 1fd5b04 to cd9ad12 Compare November 11, 2021 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants