Skip to content

Feat; Split request job into benchmark jobs #2207

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 25, 2025

Conversation

Jamesbarford
Copy link
Contributor

Adds the database table job_queue along with creating a BenchmarkJob from a BenchmarkRequest.

This does not include enqueuing a parent if there is a missing backend/profile.

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, left a few other thoughts.

I'd like to remove BenchmarkJob::create_queued, because it creates the wrong incentives - it shouldn't be usable in "normal" code, because everything should go through the DB, and it ideally shouldn't be usable in tests, because then we test something that doesn't actually happen in production - even the tests should go through the DB if possible. This is a bit different from the benchmark request, where it made sense to create it in code and then insert it into the DB, because we had several variations of the request that could exist. But the job is more specialized, and it doesn't make sense to create it outside of the database.

It would also be nice to add at least some brief doc comments in code on top of the job, just to explain what is it for.

@Jamesbarford Jamesbarford force-pushed the feat/split-jobs-and-enqueue branch from 754f63b to 21d8d31 Compare July 24, 2025 21:23
Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks! I tested this locally and noticed that multiple requests were marked as in progress, because the enqueuing loop didn't end after enqueuing the first request. I fixed that and added more logging, which helped me with debugging locally.

Btw you can run local Postgres DB, run the website with the CRON environment variables and it will actually start creating the master/release artifacts and enqueuing jobs, so it can be easily tested.

@Kobzol Kobzol force-pushed the feat/split-jobs-and-enqueue branch from 034d503 to c32b63a Compare July 25, 2025 07:46
@Kobzol
Copy link
Member

Kobzol commented Jul 25, 2025

Before we go for marking requests as complete, we should work on the collector side, as that's the next functionality in the request/job lifecycle. So there should be a new collector command, something similar to BenchNext, which will:

  • Receive the name of the collector
  • Load the collector config from the DB and update the heartbeat (could we actually do that with a single SQL query? :) )
  • Dequeue a job

I'd end there, because I still need to reland #2161 before we move forward there. I will work on that next.

@Kobzol Kobzol enabled auto-merge July 25, 2025 07:48
@Kobzol Kobzol merged commit f33ebea into rust-lang:master Jul 25, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants