- Notifications
You must be signed in to change notification settings - Fork1k
refactor(coderd): collapse activityBumpWorkspace into a single query#9652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: I view these tests as complementary toTestWorkspaceActivityBump inactivitybump_test.go; testing the end-to-end functionality here is crucial.
| workspace_builds.max_deadline::timestampAS build_max_deadline, | ||
| workspace_builds.transitionAS build_transition, | ||
| provisioner_jobs.completed_at::timestampAS job_completed_at, | ||
| (workspaces.ttl/1000/1000/1000||' seconds')::intervalAS ttl_interval, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
self-review: it probably makes sense to migrate this column to e.g.ttl_mins, but that's out of scope of this PR.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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. |
johnstcn commentedSep 13, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 thereshould be absolutely no difference. |
Uh oh!
There was an error while loading.Please reload this page.
I would say not to make it an experiment. It should have the same behavior |
| SET | ||
| updated_at= NOW(), | ||
| deadline= CASE | ||
| WHENl.build_max_deadline='0001-01-01 00:00:00' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, max deadline is+00. I'll update that to fix, but good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO there is no need to put it under experiment. If there is a problem with the routine, you can easily revert the change 👍
LGTM, I see that major comments have been addressed.
| returnks,nil | ||
| } | ||
| funcminTime(t,u time.Time) time.Time { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: if this is used only byActivityBumpWorkspace, then I would move it below/to the bottom.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#8064
Explain output for updated query (ran on big.cdr.dev):https://explain.dalibo.com/plan/365gcg47eh1dc171