Feat: Logic for finding the next request to be processed and tests #2166
Conversation
| .await; | ||
| } | ||
| } | ||
|
|
||
| /// For queueing jobs, add the jobs you want to queue to this function | ||
| async fn cron_enqueue_jobs(site_ctxt: &Arc<SiteCtxt>) { | ||
| // Put the master commits into the `benchmark_requests` queue | ||
| enqueue_master_commits(site_ctxt).await; |
There was a problem hiding this comment.
We use the term enqueue for putting things into the job queue, not into benchmark_request. So to avoid confusion, I would now rename enqueue_master_commits to something like create_master_commit_requests or something like that.
| .await; | ||
| } | ||
| } | ||
|
|
||
| /// For queueing jobs, add the jobs you want to queue to this function | ||
| async fn cron_enqueue_jobs(site_ctxt: &Arc<SiteCtxt>) { |
There was a problem hiding this comment.
Let's call let conn = ctxt.conn().await; once at the beginning of this function, and pass it to the individual subfunctions. No need to create a new DB connection or (return it back to the pool) for each function call.
| } | ||
|
|
||
| // We draw back the last 30 days of completed requests | ||
| let completed = conn |
There was a problem hiding this comment.
This should be the list of all completed benchmark requests, that we ideally preload into a cached hashmap in memory to allow fast lookups (or something like that). The 30 day limit applies to the job queue, not to the benchmark requests. In theory, someone could do a try build with a very old parent (this is supported now), which was benchmarked more than 30 days ago.
| let pending_by_sha: HashMap<String, &BenchmarkRequest> = | ||
| pending.iter().map(|r| (r.tag().to_string(), r)).collect(); | ||
|
|
||
| for req in pending.iter() { |
There was a problem hiding this comment.
Interesting, we actually don't need to build an explicit topological ordering here and do the recursive parent lookup, because we can simply take a look if the parent is completed or not. Nevertheless, we will need to build the actual full queue somewhere, because we should show it on the status page, and we also need it for estimating how long it will take to benchmark a specific PR.
|
|
||
| /// Gets the benchmark requests matching the status. Optionally provide the | ||
| /// number of days from whence to search from | ||
| async fn get_benchmark_requests_by_status( |
There was a problem hiding this comment.
I know that the old code didn't uphold this, but I'd rather avoid unwraps in new code, and explicitly return Results everywhere. Especially since the cron job is a background task, and if it panics, the whole system grinds to a halt. We might want to add some auto-restarting functionality for it, just in case.
Tees up the next request in the
benchmark_requesttable (does not create jobs in what will be thejob_queuetable.in_progressI think we should be ok? Though I can bring back the SQL version if needed.SiteCtxtthus split the sorting logic to;get_next_benchmark_request(...)waiting_for_parent, I'm a bit lost as to what the name should be so am very open to suggestions.waitingalong with some code comments about whatwaitingmeans for the different types of commits was the best I could think of however I held off