insights: add background workers which execute search queries and store insights #18267
Conversation
|
|
||
| // StartBackgroundJobs is the main entrypoint which starts background jobs for code insights. It is | ||
| // called from the repo-updater service, currently. | ||
| func StartBackgroundJobs(ctx context.Context, mainAppDB *sql.DB) { |
There was a problem hiding this comment.
Example of a function I don't think can be easily tested or refactored in a way to be easily tested.
There was a problem hiding this comment.
This is what CI/integration tests are for!
There was a problem hiding this comment.
Good point, although this does introduce a new challenge because we won't be supporting code insights in the single-container deployments. Interesting. I'll think about this some more. I wonder if we have plans to replace the single-container deployment used for CI tests with docker compose or similar.
|
Notifying subscribers in CODENOTIFY files for diff a7e4e9e...a6e95ba.
|
| insightsStore *store.Store | ||
| } | ||
|
|
||
| func (r *workHandler) Handle(ctx context.Context, workerStore dbworkerstore.Store, record workerutil.Record) (err error) { |
There was a problem hiding this comment.
Is the most reasonable way to test this mocking out every store (r.workerBaseStore, r.insightsStore, and workerStore) + the record - or just keeping this function minimal and calling into other more well-tested functions as it grows? I assume the latter, but curious if I missed a pattern we follow here.
There was a problem hiding this comment.
I try to do both in a particular way: make the handler a dumb enough driver that you can mock everything it calls and ensure it calls thing in the correct order. Then you can test each part individually. If you deconstruct it right you should have confidence in this approach (in the LSIF processor we test reading, correlation, and db writes independently and ensure that mocks are called in the correct order, not called if earlier errors occur, etc).
| } | ||
|
|
||
| // EnqueueJob enqueues a job for the query runner worker to execute later. | ||
| func EnqueueJob(ctx context.Context, workerBaseStore *basestore.Store, job *Job) (id int, err error) { |
There was a problem hiding this comment.
(Will add tests for these DB interaction methods before merging.)
|
|
||
| // Note: We run this goroutine once very 1 minute, and StalledMaxAge in queryrunner/ is | ||
| // set to 60s. If you change this, make sure the StalledMaxAge is less than this period | ||
| // otherwise there is a fair chance we could enqueue work faster than it can be completed. |
There was a problem hiding this comment.
Sounds like this comment is asking you to implement some form of backpressure. Plans or issues for that?
There was a problem hiding this comment.
Definitely, I've filed https://github.com/sourcegraph/sourcegraph/issues/18308 just now and may address this myself soon, but also plan to file a more extensive set of follow-up issues in general around the insights backend for the next person who starts work on this.
…re insights One challenge here is how to best test this code outside of an actual working dev server. Some parts (DB actions) are easy to test and I will add tests for them soon, while others are merely wiring up different components of the system so testing them is not obvioous. Stacked on top of #18265 Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
emidoots
force-pushed
the
sg/insights-worker
branch
from
08e354b to
4e1193a
Compare
Co-authored-by: Eric Fritz <eric@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
|
Added tests here for the obvious things, and am going to look at refactoring some code here (particularly the work handler) to support better testing as I build that out further. I've added a TODO on my list https://github.com/sourcegraph/sourcegraph/pull/17227 |
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
One challenge here is how to best test this code outside of an actual working dev server. Some parts (DB interactions) are easy to test and I will add tests for them soon (before merging this PR), while others are merely wiring up different components of the system so testing them is not obvious. Suggestions/advise appreciated.
Stacked on top of #18265
Signed-off-by: Stephen Gutekanst stephen@sourcegraph.com