Introduce executor.Coordinator and split up responsibilities between Coordinator/Service/executor#536
Conversation
5e73bf8 to
fd560a2
Compare
81d1153 to
5a92db3
Compare
| @@ -0,0 +1,313 @@ | |||
| package executor | |||
There was a problem hiding this comment.
Nothing changed in this file. I only moved the test here from executor_test.go
| // Update the status of Task | ||
| status.Update(taskResult.task, func(status *TaskStatus) { | ||
| status.ChangesetSpecs = specs | ||
| // TODO(mrnugget): Ideally we'd get rid of this dependency on the task |
There was a problem hiding this comment.
The reason we still have this dependency is that we log information about the diff in the TUI.
eseliger
left a comment
There was a problem hiding this comment.
Nice work! 🧹 I like how this code base is taking some nice shape :)
LawnGnome
left a comment
There was a problem hiding this comment.
I think this is a really powerful new concept! Great job! 👍
Are there any background documents on docs.sourcegraph.com that we need to update/write as a result of this?
| ) | ||
|
|
||
| func createChangesetSpecs(task *Task, result executionResult, features batches.FeatureFlags) ([]*batches.ChangesetSpec, error) { | ||
| func createChangesetSpecs(task *Task, result executionResult, autoAuthorDetails bool) ([]*batches.ChangesetSpec, error) { |
There was a problem hiding this comment.
Don't change this, but as a note to myself, I have to update #538 since it was relying on the features being drilled through to this function. 😬
| var s []*TaskStatus | ||
| for _, status := range tsc.statuses { | ||
| s = append(s, status) | ||
| } |
There was a problem hiding this comment.
Do we need to shallow copy the statuses, since the read lock is still going to be held until after callback(s) returns anyway?
There was a problem hiding this comment.
This is just copy and pasted from the old method, so I didn't chnage it. I'm not 100% sure we still need it. Will investigate in a separate branch (because I'm scared of this part of the codebase)
There was a problem hiding this comment.
Ah, the reason we do the copy of the pointers is because tsc.statuses is a map[*Task]*TaskStatus, not a slice.
Co-authored-by: Adam Harvey <aharvey@sourcegraph.com>
…Coordinator/Service/executor (#536) * Broken build with TaskStatusHubThing * Introduce Coordinator * Replace TaskStatusHubThing with TaskStatusCollection * Clean up the executor * Clean up handling of ExecutionOpts * Fix missing rename * Make executor property and status a runtime arg * Create separate tests for Coordinator * Fix tests for Coordinator * More test cleanup * Re-add missing test * Re-add transfromGroup test but its passing, what * Add test for transform group * Add test helpers * Backport fix from #539 * Update internal/batches/executor/coordinator.go Co-authored-by: Adam Harvey <aharvey@sourcegraph.com> Co-authored-by: Adam Harvey <aharvey@sourcegraph.com>
On the quest to making caching more flexible (see https://github.com/sourcegraph/sourcegraph/issues/20850) I dug into the
executor,TaskStatus,Serviceand 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 #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
executorpackage: theCoordinator. Its job is to coordinate the execution of a batch spec: kick off theexecutorthat does the actual execution ofsteps, check the cache before and save the results to the cache after, go through theimportChangesetsstatements.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
executorpackage would now beexecutionbecause it would give us:execution.NewCoordinatoras 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 allTaskStatusbehind a mutex?).