rockskip: Replace second long-running process with gRPC API#62734
Conversation
dc8d53c to
2cbe72e
Compare
|
This looks good to me!
Indeed! I have a TODO to add a couple backend integration tests for Rockskip, that will at least give some coverage here (https://github.com/sourcegraph/sourcegraph/issues/62550) Did we ever resolve @keegancsmith 's question here: https://github.com/sourcegraph/sourcegraph/pull/62260#pullrequestreview-2053060862 ? 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
left a comment
There was a problem hiding this comment.
Approved, but @sourcegraph/search-platform should review
b245a5b to
db73857
Compare
jtibshirani
left a comment
There was a problem hiding this comment.
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.
@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. |
db73857 to
3574efb
Compare
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. Closes #62100 Test plan: Existing tests still pass, I let it index a repo locally and that was fast and I get results for symbols instantly now, but not a Rockskip expert so requesting review from the owners.
3574efb to
4dda696
Compare
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.