rockskip: Replace second long-running process with gRPC API

[eseliger]

We want to migrate the LogReverseEach call to gRPC methods as well.
However, it isn't a great practice to have to keep this process running for potentially hours, as any server restart will have to make the entire process start over.

With one call per commit, we're not occupying a process slot on gitserver, and rollouts of gitserver should much less affect rockskip, as individual commits can be retried before being returned to the rockskip code.

One thing: I reimplemented what ChangedFiles does so that we can still implement integration tests on this level, but since gitserver is a separate service, a go unit test is likely long-term not the best test bed for these kinds of tests. Ideally we would spin up a small gitserver with that repo cloned, and a small program that runs the rockskip index function.
Unfortunately I think we have little precedence for these things, but that would be the highest confidence that things actually work E2E in production.

Closes #62100

Test plan:

Existing tests still pass, I let it index a repo locally and compared it to the results I got on main with a fresh index there.

[eseliger]

• gitserver: Deprecate Exec and remove localGit #62773 : 3 dependent PRs (#62774 , #62801 , #62825 )
• gitserver: Remove and deprecate no_timeout option from exec #62743
• rockskip: Replace second long-running process with gRPC API #62734 👈
• gitserver: Add CommitLog API to replace client-side Commits #62606
• gitserver: Fix duration reported for ChangedFiles #62603
• gitserver: Make ReadDir return an iterator on client side #62602
• gitserver: Move implementation of HasCommitAfter to caller #62520
• gitserver: Always use Commits in HasCommitAfter #62519
• gitserver: Implement CommitLog using Commits #62518
• gitserver: Move CommitsUniqueToBranch to callsite #62514
• gitserver: Implement CommitsUniqueToBranch using Commits #62513
• gitserver: Implement Stat and ReadDir in gRPC API #62107
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite

[jtibshirani]

This looks good to me!

Ideally we would spin up a small gitserver with that repo cloned, and a small program that runs the rockskip index function.

Indeed! I have a TODO to add a couple backend integration tests for Rockskip, that will at least give some coverage here (#62550)

Did we ever resolve @keegancsmith 's question here: #62260 (review) ? Maybe we were waiting on the perf numbers he mentioned?

[eseliger]

Did we ever resolve @keegancsmith 's question here: #62260 (review) ? Maybe we were waiting on the perf numbers he mentioned?

Ah! No, curious to hear what the numbers are :) src-cli indexed in about the same time locally vs main, but it's a very small repo.

[ggilmore]

Approved, but @sourcegraph/search-platform should review

[jtibshirani]

This looks good to me! I feel the test coverage is as good as possible given our lack of end-to-end tests -- we've covered the ChangedFiles logic, Rockskip indexing and search, and done manual checks.

[jtibshirani]

Did we ever resolve @keegancsmith 's question here: #62260 (review) ? Maybe we were waiting on the perf numbers he mentioned?

@eseliger and I chatted offline and decided we don't have a big concern here. I do think there's room to improve observability here, to be able to debug what's slow if we observe slow index times.

[eseliger]

Merge activity

• May 21, 9:22 AM EDT: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
• May 21, 11:45 AM EDT: @eseliger merged this pull request with Graphite.