Feat; mark benchmark requests as complete

[Jamesbarford]

Related to; #2200 which I've closed as it was becoming fiddly to update.

• Mark benchmark_request's as complete based on;
  • parent being complete if there is a parent
  • all jobs in the job_queue being in a terminal state.

[Jamesbarford]

This is a bit of a monster but atomically achieves what we want it to.

The query uses indexes as can be seen here through running EXPLAIN ANALYSE below.

It's verbose however the points of interest are all operations use either index scans or bitmap index scans

Update on benchmark_request  (cost=4.71..28.37 rows=1 width=52) (actual time=0.047..0.049 rows=0 loops=1)
   ->  Nested Loop Anti Join  (cost=4.71..28.37 rows=1 width=52) (actual time=0.045..0.047 rows=0 loops=1)
         ->  Index Scan using benchmark_request_tag_key on benchmark_request  (cost=0.42..17.14 rows=1 width=15) (actual time=0.045..0.046 rows=0 loops=1)
               Index Cond: (tag = 'foo'::text)
               Filter: ((status <> 'completed'::text) AND ((parent_sha IS NULL) OR (NOT (SubPlan 1))))
               SubPlan 1
                 ->  Nested Loop  (cost=0.70..16.71 rows=2 width=0) (never executed)
                       ->  Index Scan using job_queue_request_tag_idx on job_queue job_queue_1  (cost=0.28..12.25 rows=2 width=8) (never executed)
                             Index Cond: (request_tag = benchmark_request.parent_sha)
                             Filter: (status <> ALL ('{success,failure}'::text[]))
                       ->  Materialize  (cost=0.42..4.44 rows=1 width=9) (never executed)
                             ->  Index Only Scan using benchmark_request_tag_key on benchmark_request parent_request  (cost=0.42..4.44 rows=1 width=9) (never execut
ed)
                                   Index Cond: (tag = benchmark_request.parent_sha)
                                   Heap Fetches: 0
         ->  Bitmap Heap Scan on job_queue  (cost=4.30..11.21 rows=2 width=14) (never executed)
               Recheck Cond: (request_tag = 'foo'::text)
               Filter: (status <> ALL ('{success,failure}'::text[]))
               ->  Bitmap Index Scan on job_queue_request_tag_idx  (cost=0.00..4.29 rows=2 width=0) (never executed)
                     Index Cond: (request_tag = 'foo'::text)
 Planning Time: 2.288 ms
 Execution Time: 0.238 ms

[Jamesbarford]

I'm open to suggestions on how to test this. The obstacles I see are;

• Marking jobs as complete.
• A way of getting a benchmark request (or Vec of them) to see if one was marked as complete.

[Kobzol]

Looks good, thanks. I think that for testing we will need to move forward with my part (mark jobs as completed etc.). I started working on it, but it's more work than I expected, and I'm a bit stuck on reviews now.

If you want to move forward in parallel, you can prepare the functions for marking a job as successful/failed, and use them in tests for this PR.

[Kobzol]

This is not enough, because the jobs will be distinguished also by profile and codegen backend. Since the collector will always dequeue a specific job and know it's ID, we should just mark the jobs as finished based on their unique ID.

[Jamesbarford]

I see, yes. Exposing the database id in the struct would seem to make sense in this instance

[Kobzol]

Maybe create an enum BenchmarkJobConclusion { Success, Failure } to avoid the invalid states?

[Jamesbarford]

It's more a developer error I think. Much like the panic!(...) I put in update_benchmark_request_status(...) to prevent the wrong functions being used in the wrong context. And provided we have good test coverage, or good enough, we shouldn't need anything else. It was easy enough for me to fix the tests I broke.

[Jamesbarford]

After moving some code around you suggestion makes sense if we have the function prototype as;

async fn mark_benchmark_job_as_completed(
    &self,
    id: u32,
    benchmark_job_conclusion: &BenchmarkJobConclusion
) -> anyhow::Result<()>;

Then it becomes intuitive and is different to my previous comment.

[Jamesbarford]

cd7ff3d

[Kobzol]

Thank you! I just got to the point where I need job completion 😆 So gonna merge and rebase on top of this.

[fbstj]

was this intentional italics?

[Kobzol]

Probably not :) We can fix it in some follow-up.