PR Feedback; remove get_request_id(), reformat schema description, benchmark set made into a a struct

Author: Jamesbarford

database/schema.md

@@ -303,10 +303,28 @@ collector which benchmarks it should execute. The jobs will be kept in the
 table for ~30 days after being completed, so that we can quickly figure out
 what master parent jobs we need to backfill when handling try builds.
 
-```
-psql# SELECT * FROM job_queue limit 1;
+Columns:
 
-id   request_id   target    backend   benchmark_set  collector_id    created_at                   started_at                   completed_at                 status     retry   error 
----  -----------  --------  --------  -------------  --------------  ---------------------------  ---------------------------  ---------------------------  ---------  ------  --------
-23            7   AArch64   llvm      5              collector-1     2025-07-10 09:00:00.123+00   2025-07-10 09:05:02.456+00   2025-07-10 09:15:17.890+00   complete       0  
-```
+- **id** (`bigint` / `serial`): Primary-key identifier for the job row;
+  auto-increments with each new job.
+- **request_id** (`bigint`): References the parent benchmark request that
+  spawned this job.
+- **target** (`text NOT NULL`): Hardware/ISA the benchmarks must run on
+  (e.g. AArch64, x86_64).
+- **backend** (`text NOT NULL`): Code generation backend the collector should
+  test (e.g. llvm, cranelift).
+- **benchmark_set** (`int NOT NULL`): ID of the predefined benchmark suite to
+  execute.
+- **collector_id** (`text`): Id of the collector that claimed the job
+  (populated once the job is started).
+- **created_at** (`timestamptz NOT NULL`): Datetime when the job was queued.
+- **started_at** (`timestamptz`): Datetime when the collector actually began
+  running the benchmarks; NULL until the job is claimed.
+- **completed_at** (`timestampt`): Datetime when the collector finished
+  (successfully or otherwise); used to purge rows after ~30 days.
+- **status** (`text NOT NULL`): Current job state. `queued`, `in_progress`,
+  `success`, or `failure`.
+- **retry** (`int NOT NULL`): Number of times the job has been re-queued after
+  a failure; 0 on the first attempt.
+- **error** (`text`): Optional error message or stack trace from the last
+  failed run; NULL when the job succeeded.

database/src/lib.rs

@@ -1015,6 +1015,7 @@ impl BenchmarkRequestIndex {
     }
 
     pub fn add_tag(&mut self, tag: &str) {
+        self.all.insert(tag.to_string());
         self.completed.insert(tag.to_string());
     }
 }
@@ -1049,13 +1050,16 @@ impl fmt::Display for BenchmarkJobStatus {
     }
 }
 
+#[derive(Debug, Clone, PartialEq)]
+pub struct BenchmarkSet(u32);
+
 #[derive(Debug, Clone, PartialEq)]
 pub struct BenchmarkJob {
     target: Target,
     backend: CodegenBackend,
-    benchmark_set: u32,
+    benchmark_set: BenchmarkSet,
     collector_id: String,
-    created_at: Option<DateTime<Utc>>,
+    created_at: DateTime<Utc>,
     started_at: Option<DateTime<Utc>>,
     completed_at: Option<DateTime<Utc>>,
     status: BenchmarkJobStatus,
@@ -1068,14 +1072,15 @@ impl BenchmarkJob {
         backend: CodegenBackend,
         benchmark_set: u32,
         collector_id: &str,
+        created_at: DateTime<Utc>,
         status: BenchmarkJobStatus,
     ) -> Self {
         BenchmarkJob {
             target,
             backend,
-            benchmark_set,
+            benchmark_set: BenchmarkSet(benchmark_set),
             collector_id: collector_id.to_string(),
-            created_at: None,
+            created_at,
             started_at: None,
             completed_at: None,
             status,

database/src/pool.rs

@@ -213,17 +213,11 @@ pub trait Connection: Send + Sync {
 
     /// Try and mark the benchmark_request as completed. Will return `true` if
     /// it has been marked as completed else `false` meaning there was no change
-    async fn try_mark_benchmark_request_as_completed(
+    async fn mark_benchmark_request_as_completed(
         &self,
         benchmark_request: &mut BenchmarkRequest,
     ) -> anyhow::Result<bool>;
 
-    /// Given a benchmark request get the id
-    async fn get_benchmark_request_id(
-        &self,
-        benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<u32>;
-
     /// Insert a benchmark job into the `job_queue`
     async fn insert_benchmark_job(
         &self,
@@ -653,6 +647,7 @@ mod tests {
                 CodegenBackend::Llvm,
                 3,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Queued,
             );
 
@@ -673,6 +668,7 @@ mod tests {
                 CodegenBackend::Llvm,
                 3,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Queued,
             );
             let request = create_try(
@@ -716,20 +712,22 @@ mod tests {
                 CodegenBackend::Llvm,
                 1,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
             let job_2 = BenchmarkJob::new(
                 Target::X86_64UnknownLinuxGnu,
                 CodegenBackend::Llvm,
                 2,
                 "collector 2",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
 
             db_insert_jobs(&*db, request_id, &[job_1, job_2]).await;
 
             assert!(db
-                .try_mark_benchmark_request_as_completed(&mut request)
+                .mark_benchmark_request_as_completed(&mut request)
                 .await
                 .is_err());
 
@@ -761,20 +759,22 @@ mod tests {
                 CodegenBackend::Llvm,
                 1,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
             let job_2 = BenchmarkJob::new(
                 Target::X86_64UnknownLinuxGnu,
                 CodegenBackend::Llvm,
                 2,
                 "collector 2",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
 
             db_insert_jobs(&*db, request_id, &[job_1, job_2]).await;
 
             assert!(db
-                .try_mark_benchmark_request_as_completed(&mut request)
+                .mark_benchmark_request_as_completed(&mut request)
                 .await
                 .is_err());
 
@@ -807,20 +807,22 @@ mod tests {
                 CodegenBackend::Llvm,
                 1,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::InProgress,
             );
             let job_2 = BenchmarkJob::new(
                 Target::X86_64UnknownLinuxGnu,
                 CodegenBackend::Llvm,
                 2,
                 "collector 2",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
 
             db_insert_jobs(&*db, request_id, &[job_1, job_2]).await;
 
             assert!(db
-                .try_mark_benchmark_request_as_completed(&mut request)
+                .mark_benchmark_request_as_completed(&mut request)
                 .await
                 .is_ok());
             assert_eq!(request.status(), BenchmarkRequestStatus::InProgress);
@@ -853,20 +855,22 @@ mod tests {
                 CodegenBackend::Llvm,
                 1,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
             let job_2 = BenchmarkJob::new(
                 Target::X86_64UnknownLinuxGnu,
                 CodegenBackend::Llvm,
                 2,
                 "collector 2",
+                Utc::now(),
                 BenchmarkJobStatus::Success,
             );
 
             db_insert_jobs(&*db, request_id, &[job_1, job_2]).await;
 
             assert!(db
-                .try_mark_benchmark_request_as_completed(&mut request)
+                .mark_benchmark_request_as_completed(&mut request)
                 .await
                 .is_ok());
             // The struct should have been mutated
@@ -878,28 +882,4 @@ mod tests {
         })
         .await;
     }
-
-    #[tokio::test]
-    async fn get_benchmark_request_id() {
-        run_postgres_test(|ctx| async {
-            let db = ctx.db_client();
-            let db = db.connection().await;
-            let request = create_try(
-                Some("s1"),
-                Some("p1"),
-                3,
-                Utc::now(),
-                BenchmarkRequestStatus::InProgress,
-                "",
-                "",
-            );
-            db.insert_benchmark_request(&request).await.unwrap();
-
-            let retrieved = db.get_benchmark_request_id(&request).await.unwrap();
-            assert_eq!(1, retrieved);
-
-            Ok(ctx)
-        })
-        .await;
-    }
 }

database/src/pool/postgres.rs

@@ -1616,7 +1616,7 @@ where
         Ok(requests)
     }
 
-    async fn try_mark_benchmark_request_as_completed(
+    async fn mark_benchmark_request_as_completed(
         &self,
         benchmark_request: &mut BenchmarkRequest,
     ) -> anyhow::Result<bool> {
@@ -1677,43 +1677,6 @@ where
         }
     }
 
-    async fn get_benchmark_request_id(
-        &self,
-        benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<u32> {
-        anyhow::ensure!(
-            benchmark_request.tag().is_some(),
-            "Benchmark request has no tag"
-        );
-
-        let row = self
-            .conn()
-            .query_opt(
-                "
-                SELECT
-                    id
-                FROM
-                    benchmark_request
-                WHERE
-                    tag = $1
-                    AND commit_type = $2
-                    AND status = $3;",
-                &[
-                    &benchmark_request.tag(),
-                    &benchmark_request.commit_type,
-                    &benchmark_request.status,
-                ],
-            )
-            .await
-            .context("Failed to get id for benchmark_request")?;
-
-        if let Some(row) = row {
-            Ok(row.get::<_, i32>(0) as u32)
-        } else {
-            Ok(1)
-        }
-    }
-
     async fn insert_benchmark_job(
         &self,
         request_id: u32,
@@ -1735,7 +1698,7 @@ where
                     &(request_id as i32),
                     &benchmark_job.target,
                     &benchmark_job.backend,
-                    &(benchmark_job.benchmark_set as i32),
+                    &(benchmark_job.benchmark_set.0 as i32),
                     &benchmark_job.collector_id,
                     &benchmark_job.status,
                 ],

database/src/pool/sqlite.rs

@@ -1297,20 +1297,13 @@ impl Connection for SqliteConnection {
         no_queue_implementation_abort!()
     }
 
-    async fn try_mark_benchmark_request_as_completed(
+    async fn mark_benchmark_request_as_completed(
         &self,
         _benchmark_request: &mut BenchmarkRequest,
     ) -> anyhow::Result<bool> {
         no_queue_implementation_abort!()
     }
 
-    async fn get_benchmark_request_id(
-        &self,
-        _benchmark_request: &BenchmarkRequest,
-    ) -> anyhow::Result<u32> {
-        no_queue_implementation_abort!()
-    }
-
     async fn insert_benchmark_job(
         &self,
         _benchmark_request_id: u32,

site/src/job_queue/mod.rs

@@ -189,7 +189,7 @@ async fn enqueue_next_job(
             //     .await?;
             break;
         } else if conn
-            .try_mark_benchmark_request_as_completed(&mut request)
+            .mark_benchmark_request_as_completed(&mut request)
             .await?
         {
             index.add_tag(request.tag().unwrap());