Pr Feedback; more usage of &str and Option<String> for Try commit parent_sha

Author: Jamesbarford

database/src/lib.rs

@@ -843,7 +843,7 @@ pub enum BenchmarkRequestType {
     /// A Try commit
     Try {
         sha: String,
-        parent_sha: String,
+        parent_sha: Option<String>,
         pr: u32,
     },
     /// A Master commit
@@ -916,7 +916,7 @@ impl BenchmarkRequest {
 
     pub fn create_try(
         sha: &str,
-        parent_sha: &str,
+        parent_sha: Option<&str>,
         pr: u32,
         created_at: DateTime<Utc>,
         status: BenchmarkRequestStatus,
@@ -927,7 +927,7 @@ impl BenchmarkRequest {
             commit_type: BenchmarkRequestType::Try {
                 pr,
                 sha: sha.to_string(),
-                parent_sha: parent_sha.to_string(),
+                parent_sha: parent_sha.map(|it| it.to_string()),
             },
             created_at,
             completed_at: None,
@@ -984,8 +984,11 @@ impl BenchmarkRequest {
 
     pub fn parent_sha(&self) -> Option<&str> {
         match &self.commit_type {
-            BenchmarkRequestType::Try { parent_sha, .. }
-            | BenchmarkRequestType::Master { parent_sha, .. } => Some(parent_sha),
+            BenchmarkRequestType::Try { parent_sha, .. } => match parent_sha {
+                Some(parent_sha) => Some(parent_sha),
+                _ => None,
+            },
+            BenchmarkRequestType::Master { parent_sha, .. } => Some(parent_sha),
             BenchmarkRequestType::Release { tag: _ } => None,
         }
     }

database/src/pool.rs

@@ -410,7 +410,7 @@ mod tests {
 
             let try_benchmark_request = BenchmarkRequest::create_try(
                 "b-sha-2",
-                "parent-sha-2",
+                Some("parent-sha-2"),
                 32,
                 time,
                 BenchmarkRequestStatus::ArtifactsReady,
@@ -458,7 +458,7 @@ mod tests {
 
             let try_benchmark_request = BenchmarkRequest::create_try(
                 "b-sha-2",
-                "parent-sha-2",
+                Some("parent-sha-2"),
                 32,
                 time,
                 BenchmarkRequestStatus::Completed,

database/src/pool/postgres.rs

@@ -1432,8 +1432,7 @@ where
                     backends,
                     profiles
                 FROM benchmark_request
-                WHERE status = ANY($1)
-                AND (commit_type <> 'try' OR parent_sha IS NOT NULL) "
+                WHERE status = ANY($1)"
             .to_string();
 
         let rows = self
@@ -1446,45 +1445,45 @@ where
             .iter()
             .map(|row| {
                 let tag = row.get::<_, &str>(0);
-                let parent_sha = row.get::<_, Option<String>>(1);
+                let parent_sha = row.get::<_, Option<&str>>(1);
                 let pr = row.get::<_, Option<i32>>(2);
-                let commit_type = row.get::<_, String>(3);
+                let commit_type = row.get::<_, &str>(3);
                 let status = row.get::<_, BenchmarkRequestStatus>(4);
                 let created_at = row.get::<_, DateTime<Utc>>(5);
                 let completed_at = row.get::<_, Option<DateTime<Utc>>>(6);
-                let backends = row.get::<_, String>(7);
-                let profiles = row.get::<_, String>(8);
+                let backends = row.get::<_, &str>(7);
+                let profiles = row.get::<_, &str>(8);
 
-                match commit_type.as_str() {
+                match commit_type {
                     "try" => {
                         let mut try_benchmark = BenchmarkRequest::create_try(
                             tag,
-                            &parent_sha.unwrap(),
+                            parent_sha,
                             pr.unwrap() as u32,
                             created_at,
                             status,
-                            &backends,
-                            &profiles,
+                            backends,
+                            profiles,
                         );
                         try_benchmark.completed_at = completed_at;
                         try_benchmark
                     }
                     "master" => {
                         let mut master_benchmark = BenchmarkRequest::create_master(
                             tag,
-                            &parent_sha.unwrap(),
+                            parent_sha.unwrap(),
                             pr.unwrap() as u32,
                             created_at,
                             status,
-                            &backends,
-                            &profiles,
+                            backends,
+                            profiles,
                         );
                         master_benchmark.completed_at = completed_at;
                         master_benchmark
                     }
                     "release" => {
                         let mut release_benchmark = BenchmarkRequest::create_release(
-                            tag, created_at, status, &backends, &profiles,
+                            tag, created_at, status, backends, profiles,
                         );
                         release_benchmark.completed_at = completed_at;
                         release_benchmark

site/src/job_queue.rs

@@ -39,9 +39,6 @@ async fn create_benchmark_request_master_commits(
 /// Sorts try and master requests that are in the `ArtifactsReady` status.
 /// Doesn't consider in-progress requests or release artifacts.
 fn sort_benchmark_requests(done: &HashSet<String>, request_queue: &mut [BenchmarkRequest]) {
-    // A topological sort, where each "level" is additionally altered such that
-    // try commits come first, and then sorted by PR # (as a rough heuristic for
-    // earlier requests).
     let mut done: HashSet<String> = done.iter().cloned().collect();
 
     // Ensure all the items are ready to be sorted, if they are not this is
@@ -256,7 +253,7 @@ mod tests {
     fn create_try(sha: &str, parent: &str, pr: u32, age_days: &str) -> BenchmarkRequest {
         BenchmarkRequest::create_try(
             sha,
-            parent,
+            Some(parent),
             pr,
             days_ago(age_days),
             BenchmarkRequestStatus::ArtifactsReady,
@@ -366,18 +363,18 @@ mod tests {
         .await;
     }
 
-    /// Parent exists but is > 30 d old -> parent gets picked
+    /// Parent exists but is older -> parent gets picked
     #[tokio::test]
     async fn get_next_benchmark_request_oldest_first() {
         run_postgres_test(|ctx| async {
             let mut db = ctx.db_client().connection().await;
             let c1 = create_master("x", "x", 1, "days521")
                 .with_status(BenchmarkRequestStatus::Completed);
-            let c2 = create_master("y", "y", 1, "days521")
+            let c2 = create_master("y", "y", 2, "days521")
                 .with_status(BenchmarkRequestStatus::Completed);
 
-            let m1 = create_master("old", "x", 2, "days45");
-            let m2 = create_master("new", "y", 3, "days1");
+            let m1 = create_master("old", "x", 3, "days45");
+            let m2 = create_master("new", "y", 4, "days1");
 
             db_insert_requests(&*db, &[c1, c2, m1, m2]).await;
             enqueue_next_job(&mut *db).await?;
@@ -415,13 +412,13 @@ mod tests {
         run_postgres_test(|ctx| async {
             let mut db = ctx.db_client().connection().await;
             // Fresh parents that will unblock some children
-            let parent_master = create_master("parent_m", "x", 999, "days5")
+            let parent_master = create_master("parent_m", "x", 911, "days5")
                 .with_status(BenchmarkRequestStatus::Completed);
             let parent_try = create_try("parent_t", "x", 888, "days4")
                 .with_status(BenchmarkRequestStatus::Completed);
-            let parent_master_two = create_master("gp", "x", 999, "days5")
+            let parent_master_two = create_master("gp", "x", 922, "days5")
                 .with_status(BenchmarkRequestStatus::Completed);
-            let parent_master_three = create_master("blocked_p", "x", 999, "days5")
+            let parent_master_three = create_master("blocked_p", "x", 932, "days5")
                 .with_status(BenchmarkRequestStatus::Completed);
 
             // Two releases, the older one should win overall
@@ -468,21 +465,21 @@ mod tests {
         run_postgres_test(|ctx| async {
             let mut db = ctx.db_client().connection().await;
             // Fresh parents that will unblock some children
-            let parent_master = create_master("parent_m", "x", 999, "days5")
+            let parent_master = create_master("parent_m", "x", 8, "days5")
                 .with_status(BenchmarkRequestStatus::Completed);
-            let parent_try = create_try("parent_t", "x", 888, "days4")
+            let parent_try = create_try("parent_t", "x", 9, "days4")
                 .with_status(BenchmarkRequestStatus::Completed);
-            let parent_master_two = create_master("gp", "x", 999, "days5")
+            let parent_master_two = create_master("gp", "x", 10, "days5")
                 .with_status(BenchmarkRequestStatus::Completed);
-            let parent_master_three = create_master("blocked_p", "x", 999, "days5")
+            let parent_master_three = create_master("blocked_p", "x", 11, "days5")
                 .with_status(BenchmarkRequestStatus::Completed);
 
             // Ready masters (parents completed)
-            let m1 = create_master("m_low", "parent_m", 1, "days12");
+            let m1 = create_master("m_low", "parent_m", 3, "days12");
             let m2 = create_master("m_high", "parent_m", 7, "days8");
 
-            let m3 = create_master("B", "gp", 0, "days3");
-            let m4 = create_master("C", "blocked_p", 0, "days1");
+            let m3 = create_master("B", "gp", 1, "days3");
+            let m4 = create_master("C", "blocked_p", 2, "days1");
 
             // A try commit that is ready
             let t1 = create_try("t_ready", "parent_t", 42, "days2");