Make checking of cache explicit in TUI #535
Conversation
Since we use a "reference counting" mechanism when checking out archives we shouldn't bump the ref count if we never execute steps. Because only after executing the steps do we decrease the ref count and thus "free" the archive to be cleaned up. The problem is that if a `Task` has a cache hit then the ref will never be decreased and the archive never cleaned up.
| "github.com/sourcegraph/src-cli/internal/batches" | ||
| ) | ||
|
|
||
| func CheckCache(ctx context.Context, cache ExecutionCache, clearCache bool, features batches.FeatureFlags, task *Task) (specs []*batches.ChangesetSpec, found bool, err error) { |
There was a problem hiding this comment.
This is Not Good ™️ and I want to refactor this into another entity/thing.
There was a problem hiding this comment.
What are you thinking here?
(I wrote this before I got to your musings on the fields currently in the Service struct, but if there's more than just that, I'm interested!)
There was a problem hiding this comment.
I'm not sure yet. I'm playing around with some ideas in another branch. The big problem is the TaskStatuses and how they're woven into the executor etc.
| "github.com/sourcegraph/src-cli/internal/batches" | ||
| ) | ||
|
|
||
| func createChangesetSpecs(task *Task, result executionResult, features batches.FeatureFlags) ([]*batches.ChangesetSpec, error) { |
There was a problem hiding this comment.
Nothing changed here. Everything in this file is moved out of executor.go. It's all related to building/rendering changeset specs.
| return | ||
| } | ||
|
|
||
| // Build the changeset specs. |
There was a problem hiding this comment.
This has been moved down so that everything related to the spec-building is close together.
|
|
||
| // Check if the task is cached. | ||
| cacheKey := task.cacheKey() | ||
| if x.clearCache { |
There was a problem hiding this comment.
This removed section is now in the CheckCache function
| @@ -0,0 +1,40 @@ | |||
| package executor | |||
There was a problem hiding this comment.
Nothing new in this file. Task has been moved here out of executor.go
| @@ -0,0 +1,82 @@ | |||
| package executor | |||
There was a problem hiding this comment.
Nothing new here. TaskStatus has been moved here out of executor.go
|
|
||
| // TODO(mrnugget): I don't like this state here, ugh. | ||
| exec executor.Executor | ||
| cache executor.ExecutionCache |
There was a problem hiding this comment.
If we have a separate entity that can do the whole check-cache+build-specs stuff, then we don't need this anymore.
There was a problem hiding this comment.
This looks reasonable to me. I guess the more process question in my mind here is whether you consider this releasable in its current state, given you want to clean this up further: if not, should we merge this to main, or have a (very) short lived feature branch for this and your further cleanups that we can review piecemeal, then merge back to main?
| "github.com/sourcegraph/src-cli/internal/batches" | ||
| ) | ||
|
|
||
| func CheckCache(ctx context.Context, cache ExecutionCache, clearCache bool, features batches.FeatureFlags, task *Task) (specs []*batches.ChangesetSpec, found bool, err error) { |
There was a problem hiding this comment.
What are you thinking here?
(I wrote this before I got to your musings on the fields currently in the Service struct, but if there's more than just that, I'm interested!)
Yeah, I get the hestitation. And I agree, this is not ideal. But I've thought about this a bunch (before opening the PR and after) and I think in this case I'd rather merge this to Edit: I'm going to go ahead and merge this, but if you feel strongly about this - which I can totally understand - then we can revert it and do the feature-branch. And that's not a problem. It's an easy-to-do option we have. |
* Only checkout repo archive if steps will be executed Since we use a "reference counting" mechanism when checking out archives we shouldn't bump the ref count if we never execute steps. Because only after executing the steps do we decrease the ref count and thus "free" the archive to be cleaned up. The problem is that if a `Task` has a cache hit then the ref will never be decreased and the archive never cleaned up. * WIP is working * More tiny refactoring * Working state * Move stuff around * More refactoring * Improve caching messages after feedback
This is based on @eseliger's idea, which was confirmed independently by @courier-new as something worthwhile.
In short: before this change we wouldn't tell users whether we have cached results and we'd only check the check once we start executing a task. The problem with that is that if you have 500 tasks to execute, 480 of which are cached, the UI would tell you "executing 4/500 tasks" and then only speed up once it gets to the cached values.
This PR here pulls the cache-checking out of the task execution and makes it a separate step in the batch spec execution and in the UI:
Note to reviewers: there is also a lot of other clean-up/refactoring/move-around stuff in here that I either (a) had to do to make this easier or (b) wanted to do after digging more into this and realizing that we could probably decouple the executor from this whole process of caching/building-changeset-specs.
I'm not done yet, but I decided to open this PR before things get unreviewable.
Take a look at 3e8475d to see the gist of this change. The other commits mostly move things in separate files, rename stuff, clean up dependency chains.
I'm going to create a new branch off of this one for more cleanups/refactorings.