refactor(coderd): collapse activityBumpWorkspace into a single query #9652
Conversation
There was a problem hiding this comment.
self-review: I can inline this CTE if required, but I think the readability is important here. I'm preserving the comments from the original pure-Go implementation for reference.
coderd/activitybump_internal_test.go
Outdated
There was a problem hiding this comment.
self-review: I view these tests as complementary to TestWorkspaceActivityBump in activitybump_test.go; testing the end-to-end functionality here is crucial.
| workspace_builds.max_deadline::timestamp AS build_max_deadline, | ||
| workspace_builds.transition AS build_transition, | ||
| provisioner_jobs.completed_at::timestamp AS job_completed_at, | ||
| (workspaces.ttl / 1000 / 1000 / 1000 || ' seconds')::interval AS ttl_interval, |
There was a problem hiding this comment.
self-review: it probably makes sense to migrate this column to e.g. ttl_mins, but that's out of scope of this PR.
|
Poll: should I make this an experiment? |
If I understood correctly, logic changes here. So I guess it depends on if this is a bugfix or improvement. If it's the latter this could be an experiment or breaking change. If it's the former I guess still breaking but no need to put it behind experiment. |
It's a refactor, so there should be absolutely no functional changes. I made sure that the tests I added covered the existing logic before modifying the logic. So there should be absolutely no difference. |
|
I would say not to make it an experiment. It should have the same behavior |
| SET | ||
| updated_at = NOW(), | ||
| deadline = CASE | ||
| WHEN l.build_max_deadline = '0001-01-01 00:00:00' |
There was a problem hiding this comment.
I think, maybe, this actually should be in +00 format (UTC), but it depends on what our default value is and DB being in non-UTC config. I imagine this is something we may be doing (potentially) incorrectly elsewhere too so maybe doesn't need fixing in this PR.
There was a problem hiding this comment.
Ah, max deadline is +00. I'll update that to fix, but good catch.
| @@ -685,6 +685,13 @@ func (q *FakeQuerier) GetActiveDBCryptKeys(_ context.Context) ([]database.DBCryp | |||
| return ks, nil | |||
| } | |||
|
|
|||
| func minTime(t, u time.Time) time.Time { | |||
There was a problem hiding this comment.
nit: if this is used only by ActivityBumpWorkspace, then I would move it below/to the bottom.
Fixes #8064
Explain output for updated query (ran on big.cdr.dev): https://explain.dalibo.com/plan/365gcg47eh1dc171