Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

searcher: use the same gitserver fetch timeout as symbols#47469

Merged
keegancsmith merged 2 commits into
mainfrom
k/store-status
Feb 9, 2023
Merged

searcher: use the same gitserver fetch timeout as symbols#47469
keegancsmith merged 2 commits into
mainfrom
k/store-status

Conversation

@keegancsmith

Copy link
Copy Markdown
Member

Searcher has a hardcoded timeout of 10 minutes. Interestingly symbols has a hardcoded timeout of 2 hours. Now this is likely due to fetching more history for a repository for rockskip. However, I suspect 10 minutes is too aggressive for large monorepos. As such we use a much more conservative timeout of 2h.

The downside of this approach is we have a relatively naive semaphore controlling concurrent fetches. So if we have a large burst of activity that never gets retried it will take a lot longer for us to cancel out the backlog. However, in practice this feels like the better tradeoff as well as this being consistent with symbols.

Test Plan: go test

Searcher has a hardcoded timeout of 10 minutes. Interestingly symbols
has a hardcoded timeout of 2 hours. Now this is likely due to fetching
more history for a repository for rockskip. However, I suspect 10
minutes is too aggressive for large monorepos. As such we use a much
more conservative timeout of 2h.

The downside of this approach is we have a relatively naive semaphore
controlling concurrent fetches. So if we have a large burst of activity
that never gets retried it will take a lot longer for us to cancel out
the backlog. However, in practice this feels like the better tradeoff as
well as this being consistent with symbols.

Test Plan: go test
@keegancsmith keegancsmith requested review from a team February 9, 2023 08:29
@cla-bot cla-bot Bot added the cla-signed label Feb 9, 2023
@keegancsmith

Copy link
Copy Markdown
Member Author

@chwarwick I got started on implementing the stub API I promised, but noticed this along the way. This may be the root cause of why we never end up processing historical insights on large monorepos sometimes.

@chwarwick

Copy link
Copy Markdown
Contributor

@chwarwick I got started on implementing the stub API I promised, but noticed this along the way. This may be the root cause of why we never end up processing historical insights on large monorepos sometimes.

Thanks @keegancsmith. Do you have any recommendations about what would typically be the best way to parallelize the insights historic searches? We can pretty easily change between the following:

  • Multiple repos in parallel, searching 1 revision at a time
  • A single repo running searches for multiple revisions in parallel
  • Multiple repos in parallel running searches for multiple revisions at in parallel.

@keegancsmith

Copy link
Copy Markdown
Member Author

@chwarwick I got started on implementing the stub API I promised, but noticed this along the way. This may be the root cause of why we never end up processing historical insights on large monorepos sometimes.

Thanks @keegancsmith. Do you have any recommendations about what would typically be the best way to parallelize the insights historic searches? We can pretty easily change between the following:

  • Multiple repos in parallel, searching 1 revision at a time
  • A single repo running searches for multiple revisions in parallel
  • Multiple repos in parallel running searches for multiple revisions at in parallel.

There is no good answer here since it depends on how many searchers they have/etc. I would make your semaphore scale based on the number of searchers there are. This is something we do today https://sourcegraph.com/github.com/sourcegraph/sourcegraph@51442394e48f51334677bc91d69fbf4a7909e76f/-/blob/internal/search/searcher/search.go?L85-92

Let N be the number of searchers. Then I would do multiple repos in parallel, searching N revision at a time.

@keegancsmith keegancsmith enabled auto-merge (squash) February 9, 2023 12:43
@keegancsmith keegancsmith merged commit 46a2c89 into main Feb 9, 2023
@keegancsmith keegancsmith deleted the k/store-status branch February 9, 2023 12:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants