fix(coderd/database): exclude canceled jobs in queue position #15835
Conversation
…ProvisionerJobsByIDsWithQueuePosition
| @@ -50,23 +50,29 @@ WHERE | |||
| id = ANY(@ids :: uuid [ ]); | |||
|
|
|||
| -- name: GetProvisionerJobsByIDsWithQueuePosition :many | |||
| WITH unstarted_jobs AS ( | |||
| WITH pending_jobs AS ( | |||
There was a problem hiding this comment.
review: Renamed for clarity.
| AND | ||
| canceled_at IS NULL | ||
| AND | ||
| completed_at IS NULL | ||
| AND | ||
| error IS NULL |
There was a problem hiding this comment.
review: this is in line with the generated provisioner job status:
CASE
WHEN (completed_at IS NOT NULL) THEN
CASE
WHEN (error <> ''::text) THEN 'failed'::provisioner_job_status
WHEN (canceled_at IS NOT NULL) THEN 'canceled'::provisioner_job_status
ELSE 'succeeded'::provisioner_job_status
END
ELSE
CASE
WHEN (error <> ''::text) THEN 'failed'::provisioner_job_status
WHEN (canceled_at IS NOT NULL) THEN 'canceling'::provisioner_job_status
WHEN (started_at IS NULL) THEN 'pending'::provisioner_job_status
ELSE 'running'::provisioner_job_status
END
END)| AND | ||
| completed_at IS NULL | ||
| AND | ||
| error IS NULL | ||
| ), | ||
| queue_position AS ( | ||
| SELECT | ||
| id, | ||
| ROW_NUMBER() OVER (ORDER BY created_at ASC) AS queue_position |
There was a problem hiding this comment.
We'll need to amend this to take into account matching tags. Queue position could be 1/3 for the provisioners that match, but 20/50 on a larger scale (but that's nonsensical). Do we have an issue that tracks this?
There was a problem hiding this comment.
Account matching tags? Is that provisioner tags?
There was a problem hiding this comment.
Take into account 😄. Matching provisioner tags yes 👍🏻
There was a problem hiding this comment.
Will create a follow-up issue to track this. 👍
Edit: #15843
| @@ -3804,35 +3804,92 @@ func (q *FakeQuerier) GetProvisionerJobsByIDsWithQueuePosition(_ context.Context | |||
| q.mutex.RLock() | |||
| defer q.mutex.RUnlock() | |||
|
|
|||
| jobs := make([]database.GetProvisionerJobsByIDsWithQueuePositionRow, 0) | |||
| queuePosition := int64(1) | |||
| // WITH pending_jobs AS ( | |||
There was a problem hiding this comment.
This is lovely, but I can't wait to get rid of dbmem.
There was a problem hiding this comment.
I have a love/hate relationship with dbmem. Part of me will be a bit sad 😆
There was a problem hiding this comment.
I’m with you @Emyrk, many times it has been a nice way to verify query logic/results by forcing you to reproduce the logic in an alternative way. Still going to be happy that it’s gone though 🥲
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
When calculating the queue position in
GetProvisionerJobsByIDsWithQueuePositionwe only counted jobs withstarted_at = NULL. This is misleading, as it allows canceling or canceled jobs to take up rows in the computed queue position, giving an impression that the queue is larger than it really is.This modifies the query to also exclude jobs with a null
canceled_at,completed_at, orerrorfield for the purposes of calculating the queue position, and also adds a test to validate this behaviour.(Note: due to the behaviour of
dbgen.ProvisionerJobwithdbmemI had to use other proxy methods to validate the corresponding dbmem implementation.)