Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix(coderd/database): consider tag sets when calculating queue position#16685

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

Merged
evgeniy-scherbina merged 20 commits intomainfrom15843-queue-position
Mar 3, 2025

Conversation

@evgeniy-scherbina
Copy link
Contributor

@evgeniy-scherbinaevgeniy-scherbina commentedFeb 24, 2025•
edited
Loading

Relates to#15843

PR Contents

  • Reimplementation of theGetProvisionerJobsByIDsWithQueuePosition SQL query totake into account provisioner job tags and provisioner daemon tags.
  • Unit tests covering differenttag sets,job statuses, andjob ordering scenarios.

Notes

  • The original row order is preserved by introducing theordinality field.
  • Unnecessary rows are filtered as early as possible to ensure that expensive joins operate on a smaller dataset.
  • A "fake" join withprovisioner_jobs is added at the end to ensuresqlc.embed compiles successfully.
  • Backward compatibility is preserved—only the SQL query has been updated, while the Go code remains unchanged.

@evgeniy-scherbinaevgeniy-scherbina changed the titlereimplement GetProvisionerJobsByIDsWithQueuePosition queryfix(coderd/database): consider tag sets when calculating queue positionFeb 24, 2025
@evgeniy-scherbinaevgeniy-scherbina marked this pull request as ready for reviewFebruary 25, 2025 19:27
Copy link
Member

@EmyrkEmyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 matchingany provisioner
  • 0 jobs & 0 provisioners
  • N jobs with at least 1 being completed (not pending)

Just want to cover all cases

Comment on lines 89 to 90
COALESCE(MIN(rj.queue_position),0) ::BIGINTAS queue_position,-- Best queue position across provisioners
COALESCE(MAX(rj.queue_size),0) ::BIGINTAS queue_size,-- Max queue size across provisioners
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I understand this0. Should we instead use-1 to indicate no validqueue_position orqueue_size?

0 could be misinterpreted as "complete" imo.-1 is more obviously "not in a queue".

@evgeniy-scherbinaevgeniy-scherbinaforce-pushed the15843-queue-position branch 2 times, most recently from59cd0fd to29048ecCompareFebruary 26, 2025 21:06
@evgeniy-scherbina
Copy link
ContributorAuthor

@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@ids too early, meaning ifjobID is not passed toGetProvisionerJobsByIDsWithQueuePosition it won't be considered duringqueue_pos &queue_size calculation - so I fixed it and added test cases to cover it, they use this approach:

// 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.skipJobIDsmap[int]struct{}

Copy link
Member

@EmyrkEmyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM! 👍

Emyrk
Emyrk previously requested changesFeb 28, 2025
Copy link
Member

@EmyrkEmyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry forgot about dbmem. 😢

SQL and tests look good 👍

@evgeniy-scherbinaevgeniy-scherbinaforce-pushed the15843-queue-position branch 2 times, most recently from45f8bc6 toaee0a11CompareFebruary 28, 2025 22:43
@@ -0,0 +1 @@
CREATEINDEXidx_provisioner_jobs_statusON provisioner_jobs USING btree (job_status);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Review: it took me a minute to figure out if this is supported, but apparently it is! Thedocs on generated columns specify that only stored generated columns are supported, and these are always written to disk. TIL!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@johnstcn
yes, that's what I figured as well
Also I thought about usingHash Index instead ofBTree, because so far we're only using= or!= operations, but wasn't sure, maybe we'll need other operations in the future.

Copy link
Member

@johnstcnjohnstcnMar 3, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

evgeniy-scherbina reacted with thumbs up emoji
name:"test-case-12",
jobTags: []database.StringMap{},
daemonTags: []database.StringMap{},
queueSizes:nil,// TODO(yevhenii): should it be empty array instead?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 nevernull. So anil slice -->[] on the typescript side of things.

readonlyaudit_logs:readonlyAuditLog[];

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Obligatory reminder to check migration number before merge!

evgeniy-scherbina reacted with thumbs up emoji
@johnstcnjohnstcn dismissedEmyrk’sstale reviewMarch 3, 2025 14:29

dbmem is done now

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍 Changes look good to me! Nice tests!

@evgeniy-scherbinaevgeniy-scherbina merged commitb85ba58 intomainMar 3, 2025
30 checks passed
@evgeniy-scherbinaevgeniy-scherbina deleted the 15843-queue-position branchMarch 3, 2025 15:02
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMar 3, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@EmyrkEmyrkEmyrk approved these changes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@evgeniy-scherbina@johnstcn@Emyrk
[8]ページ先頭
©2009-2025 Movatter.jp