fix(coderd/database): consider tag sets when calculating queue position #16685
Conversation
7b52e56 to
d863758
Compare
d863758 to
5c49221
Compare
There was a problem hiding this comment.
This is looking good, the SQL query is compelx, but well structured.
Can we ensure we have unit tests for the following cases:
- N jobs (1 job with 0 tags) & 0 provisioners exist
- N jobs (1 with 0 tags) & N provisioners
- 1 job not matching any provisioner
- 0 jobs & 0 provisioners
- N jobs with at least 1 being completed (not pending)
Just want to cover all cases
| COALESCE(MIN(rj.queue_position), 0) :: BIGINT AS queue_position, -- Best queue position across provisioners | ||
| COALESCE(MAX(rj.queue_size), 0) :: BIGINT AS queue_size, -- Max queue size across provisioners |
There was a problem hiding this comment.
I understand this 0. Should we instead use -1 to indicate no valid queue_position or queue_size?
0 could be misinterpreted as "complete" imo. -1 is more obviously "not in a queue".
59cd0fd to
29048ec
Compare
29048ec to
acb93ac
Compare
|
@Emyrk I added test cases according to your recommendations: e7693ee Also in previous version I made a mistake, I filtered out provisioner jobs by passed // GetProvisionerJobsByIDsWithQueuePosition takes jobIDs as a parameter.
// If skipJobIDs is empty, all jobs are passed to the function; otherwise, the specified jobs are skipped.
// NOTE: Skipping job IDs means they will be excluded from the result,
// but this should not affect the queue position or queue size of other jobs.
skipJobIDs map[int]struct{} |
fc07f50 to
b4ea4db
Compare
4e5f152 to
3014491
Compare
3014491 to
61a9f58
Compare
45f8bc6 to
aee0a11
Compare
aee0a11 to
4f77f67
Compare
| @@ -0,0 +1 @@ | |||
| CREATE INDEX idx_provisioner_jobs_status ON provisioner_jobs USING btree (job_status); | |||
There was a problem hiding this comment.
Review: it took me a minute to figure out if this is supported, but apparently it is! The docs on generated columns specify that only stored generated columns are supported, and these are always written to disk. TIL!
There was a problem hiding this comment.
@johnstcn
yes, that's what I figured as well
Also I thought about using Hash Index instead of BTree, because so far we're only using = or != operations, but wasn't sure, maybe we'll need other operations in the future.
There was a problem hiding this comment.
TBH this is probably fine, almost all of our other indexes are using BTree. If you were curious, I suppose you could validate with some EXPLAINs on a sample dataset.
| name: "test-case-12", | ||
| jobTags: []database.StringMap{}, | ||
| daemonTags: []database.StringMap{}, | ||
| queueSizes: nil, // TODO(yevhenii): should it be empty array instead? |
There was a problem hiding this comment.
In general I prefer to return an empty slice for something that is always expected to be present. However, as these responses end up being converted to Typescript objects, we kind of always have to specify arrays as being nullable due to Go's type system. @Emyrk may have some more thoughts here.
There was a problem hiding this comment.
This is for a test case. I also generally prefer an empty slice in this case for the equality assertion. But I have no strong opinion in this test case.
For the typescript side of things, (off the top of my head an quick glancing) slices are never null. So a nil slice --> [] on the typescript side of things.
coder/site/src/api/typesGenerated.ts
Line 185 in 6f4da84
There was a problem hiding this comment.
Obligatory reminder to check migration number before merge!
Relates to #15843
PR Contents
GetProvisionerJobsByIDsWithQueuePositionSQL query to take into account provisioner job tags and provisioner daemon tags.Notes
ordinalityfield.provisioner_jobsis added at the end to ensuresqlc.embedcompiles successfully.