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

Jamesbarford · Jamesbarford · commit eb422a5164c6 · 2025-07-30T15:35:23.000+01:00
diff --git a/database/schema.md b/database/schema.md
@@ -321,11 +321,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  
-```
->>>>>>> d0ed82ff (Feat; job_queue table definition & mark a request as complete if all jobs are finished)
+- **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.
diff --git a/database/src/lib.rs b/database/src/lib.rs
@@ -1053,6 +1053,7 @@ impl BenchmarkRequestIndex {
     }
 
     pub fn add_tag(&mut self, tag: &str) {
+        self.all.insert(tag.to_string());
         self.completed.insert(tag.to_string());
     }
 }
@@ -1087,13 +1088,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,
@@ -1106,14 +1110,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,
diff --git a/database/src/pool.rs b/database/src/pool.rs
@@ -235,16 +235,10 @@ 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>;
 }
 
 #[async_trait::async_trait]
@@ -762,6 +756,7 @@ mod tests {
                 CodegenBackend::Llvm,
                 3,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Queued,
             );
 
@@ -782,6 +777,7 @@ mod tests {
                 CodegenBackend::Llvm,
                 3,
                 "collector 1",
+                Utc::now(),
                 BenchmarkJobStatus::Queued,
             );
             let request = create_try(
@@ -825,20 +821,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());
 
@@ -870,20 +868,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());
 
@@ -916,20 +916,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);
@@ -962,20 +964,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
@@ -987,28 +991,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;
-    }
 }
diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs
@@ -1,16 +1,6 @@
 use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction};
 use crate::selector::CompileTestCase;
 use crate::{
-<<<<<<< HEAD
-    ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkJobStatus,
-||||||| parent of d0ed82ff (Feat; job_queue table definition & mark a request as complete if all jobs are finished)
-    ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkRequest,
-    BenchmarkRequestIndex, BenchmarkRequestStatus, BenchmarkRequestType, CodegenBackend,
-    CollectionId, Commit, CommitType, CompileBenchmark, Date, Index, Profile, QueuedCommit,
-    Scenario, Target, BENCHMARK_REQUEST_MASTER_STR, BENCHMARK_REQUEST_RELEASE_STR,
-=======
-    ArtifactCollection, ArtifactId, ArtifactIdNumber, Benchmark, BenchmarkJob, BenchmarkJobStatus,
->>>>>>> d0ed82ff (Feat; job_queue table definition & mark a request as complete if all jobs are finished)
     BenchmarkRequest, BenchmarkRequestIndex, BenchmarkRequestStatus, BenchmarkRequestType,
     CodegenBackend, CollectionId, Commit, CommitType, CompileBenchmark, Date, Index, Profile,
     QueuedCommit, Scenario, Target, BENCHMARK_REQUEST_MASTER_STR, BENCHMARK_REQUEST_RELEASE_STR,
@@ -1721,7 +1711,7 @@ where
             .collect())
     }
 
-    async fn try_mark_benchmark_request_as_completed(
+    async fn mark_benchmark_request_as_completed(
         &self,
         benchmark_request: &mut BenchmarkRequest,
     ) -> anyhow::Result<bool> {
@@ -1781,43 +1771,6 @@ where
             Ok(false)
         }
     }
-
-    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)
-        }
-    }
 }
 
 fn parse_artifact_id(ty: &str, sha: &str, date: Option<DateTime<Utc>>) -> ArtifactId {
diff --git a/database/src/pool/sqlite.rs b/database/src/pool/sqlite.rs
@@ -1,9 +1,9 @@
 use crate::pool::{Connection, ConnectionManager, ManagedConnection, Transaction};
 use crate::selector::CompileTestCase;
 use crate::{
-    ArtifactCollection, ArtifactId, Benchmark, BenchmarkJob, BenchmarkRequest,
-    BenchmarkRequestIndex, BenchmarkRequestStatus, CodegenBackend, CollectionId, Commit,
-    CommitType, CompileBenchmark, Date, Profile, Target,
+    ArtifactCollection, ArtifactId, Benchmark, BenchmarkRequest, BenchmarkRequestIndex,
+    BenchmarkRequestStatus, CodegenBackend, CollectionId, Commit, CommitType, CompileBenchmark,
+    Date, Profile, Target,
 };
 use crate::{ArtifactIdNumber, Index, QueuedCommit};
 use chrono::{DateTime, TimeZone, Utc};
@@ -1331,19 +1331,12 @@ impl Connection for SqliteConnection {
             .collect::<Result<_, _>>()?)
     }
 
-    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!()
-    }
 }
 
 fn parse_artifact_id(ty: &str, sha: &str, date: Option<i64>) -> ArtifactId {
