Skip to content

Introduce executor.Coordinator and split up responsibilities between Coordinator/Service/executor

Warren Gifford requested to merge mrn/refactor-statuses into main

Created by: mrnugget

On the quest to making caching more flexible (see https://github.com/sourcegraph/sourcegraph/issues/20850) I dug into the executor, TaskStatus, Service and how all the things fit together. This is the result of that digging. Hopefully it improves the clarity in the codebase by pulling apart some of the things that have been mixed together over time: caching, execution of steps, updating of task statuses.

It's also a follow-up to https://github.com/sourcegraph/src-cli/pull/535 in which I left a bunch of TODOs lying around. These are now gone.

And it also contains @eseliger's fix from #539.

Now, what's in here?

I introduced another entity in the executor package: the Coordinator. Its job is to coordinate the execution of a batch spec: kick off the executor that does the actual execution of steps, check the cache before and save the results to the cache after, go through the importChangesets statements.

This separation lets us write clearer tests: the executor tests are now only about the execution of steps and the coordinator tests now test the creation and caching of changeset specs, which the executor knows nothing about.

(Sidenote: a better name for the executor package would now be execution because it would give us: execution.NewCoordinator as identifier)

Out of necessity I also had to change how the updating of TaskStatuses works. I have some more ideas for the future (how about updates to a task are published on a channel instead of sharing the whole slice of all TaskStatus behind a mutex?).

Merge request reports