Merge pull request #2196 from Jamesbarford/chore/modify-try-commit-type

Chore; make `tag` optional for Try benchmark requests

Author: Kobzol

database/src/lib.rs

@@ -842,7 +842,7 @@ impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus {
 pub enum BenchmarkRequestType {
     /// A Try commit
     Try {
-        sha: String,
+        sha: Option<String>,
         parent_sha: Option<String>,
         pr: u32,
     },
@@ -915,7 +915,7 @@ impl BenchmarkRequest {
     }
 
     pub fn create_try(
-        sha: &str,
+        sha: Option<&str>,
         parent_sha: Option<&str>,
         pr: u32,
         created_at: DateTime<Utc>,
@@ -926,7 +926,7 @@ impl BenchmarkRequest {
         Self {
             commit_type: BenchmarkRequestType::Try {
                 pr,
-                sha: sha.to_string(),
+                sha: sha.map(|it| it.to_string()),
                 parent_sha: parent_sha.map(|it| it.to_string()),
             },
             created_at,
@@ -962,10 +962,11 @@ impl BenchmarkRequest {
 
     /// Get either the `sha` for a `try` or `master` commit or a `tag` for a
     /// `release`
-    pub fn tag(&self) -> &str {
+    pub fn tag(&self) -> Option<&str> {
         match &self.commit_type {
-            BenchmarkRequestType::Try { sha, .. } | BenchmarkRequestType::Master { sha, .. } => sha,
-            BenchmarkRequestType::Release { tag } => tag,
+            BenchmarkRequestType::Try { sha, .. } => sha.as_deref(),
+            BenchmarkRequestType::Master { sha, .. } => Some(sha),
+            BenchmarkRequestType::Release { tag } => Some(tag),
         }
     }
 
@@ -984,10 +985,7 @@ impl BenchmarkRequest {
 
     pub fn parent_sha(&self) -> Option<&str> {
         match &self.commit_type {
-            BenchmarkRequestType::Try { parent_sha, .. } => match parent_sha {
-                Some(parent_sha) => Some(parent_sha),
-                _ => None,
-            },
+            BenchmarkRequestType::Try { parent_sha, .. } => parent_sha.as_deref(),
             BenchmarkRequestType::Master { parent_sha, .. } => Some(parent_sha),
             BenchmarkRequestType::Release { tag: _ } => None,
         }

database/src/pool.rs

@@ -409,7 +409,7 @@ mod tests {
             );
 
             let try_benchmark_request = BenchmarkRequest::create_try(
-                "b-sha-2",
+                Some("b-sha-2"),
                 Some("parent-sha-2"),
                 32,
                 time,
@@ -457,7 +457,7 @@ mod tests {
             );
 
             let try_benchmark_request = BenchmarkRequest::create_try(
-                "b-sha-2",
+                Some("b-sha-2"),
                 Some("parent-sha-2"),
                 32,
                 time,

database/src/pool/postgres.rs

@@ -301,6 +301,11 @@ static MIGRATIONS: &[&str] = &[
     );
     CREATE INDEX IF NOT EXISTS benchmark_request_status_idx on benchmark_request (status) WHERE status != 'completed';
     "#,
+    // Remove that tag cannot be NULL
+    r#"ALTER TABLE benchmark_request ALTER COLUMN tag DROP NOT NULL;"#,
+    // Prevent multiple try commits without a `sha` and the same `pr` number
+    // being added to the table
+    r#"CREATE UNIQUE INDEX benchmark_request_pr_commit_type_idx ON benchmark_request (pr, commit_type) WHERE status != 'completed';"#,
 ];
 
 #[async_trait::async_trait]
@@ -1457,7 +1462,7 @@ where
                 match commit_type {
                     "try" => {
                         let mut try_benchmark = BenchmarkRequest::create_try(
-                            tag,
+                            Some(tag),
                             parent_sha,
                             pr.unwrap() as u32,
                             created_at,

site/src/job_queue.rs

@@ -182,7 +182,16 @@ fn sort_benchmark_requests(done: &HashSet<String>, request_queue: &mut [Benchmar
             )
         });
         for c in level {
-            done.insert(c.tag().to_string());
+            // As the only `commit_type` that will not have a `tag` is a `Try`
+            // with the status of `AwaitingArtifacts` and we have asserted above
+            // that all of the statuses of the benchmark requests are
+            // `ArtifactsReady` it is implausable for this `expect(...)` to be
+            // hit.
+            done.insert(
+                c.tag()
+                    .expect("Tag should exist on a benchmark request being sorted")
+                    .to_string(),
+            );
         }
         finished += level_len;
     }
@@ -241,7 +250,7 @@ pub async fn build_queue(
 
     release_artifacts.sort_unstable_by(|a, b| {
         a.tag()
-            .cmp(b.tag())
+            .cmp(&b.tag())
             .then_with(|| a.created_at.cmp(&b.created_at))
     });
 
@@ -258,7 +267,7 @@ async fn enqueue_next_job(conn: &mut dyn database::pool::Connection) -> anyhow::
         .get_benchmark_requests_by_status(&[BenchmarkRequestStatus::Completed])
         .await?
         .into_iter()
-        .map(|request| request.tag().to_string())
+        .filter_map(|request| request.tag().map(|tag| tag.to_string()))
         .collect();
 
     let queue = build_queue(conn, &completed).await?;
@@ -360,7 +369,7 @@ mod tests {
 
     fn create_try(sha: &str, parent: &str, pr: u32, age_days: &str) -> BenchmarkRequest {
         BenchmarkRequest::create_try(
-            sha,
+            Some(sha),
             Some(parent),
             pr,
             days_ago(age_days),
@@ -390,7 +399,10 @@ mod tests {
     }
 
     fn queue_order_matches(queue: &[BenchmarkRequest], expected: &[&str]) {
-        let queue_shas: Vec<&str> = queue.iter().map(|req| req.tag()).collect();
+        let queue_shas: Vec<&str> = queue
+            .iter()
+            .filter_map(|request| request.tag().map(|tag| tag))
+            .collect();
         assert_eq!(queue_shas, expected)
     }
 
@@ -423,23 +435,23 @@ mod tests {
              *             +------------+
              *             | r "v1.2.3" |
              *             +------------+
+             *
+             *
+             *
              *                                  1: Currently `in_progress`
-             *                                     +---------------+
-             *                                +--->| t "t1" IP pr1 |
-             *                                |    +---------------+
-             *             +-----------+      |
-             *             | m "rrr" C | -----+-->
-             *             +-----------+      |
-             *                                |    +---------------+
-             *                                +--->| t "yee" R pr1 | 3: a try with a low pr
-             *                                     +---------------+
+             *             +-----------+           +---------------+
+             *             | m "rrr" C | -----+--->| t "t1" IP pr1 |
+             *             +-----------+           +---------------+
+             *
+             *
+             *
              *             +-----------+
              *             | m "aaa" C |
              *             +-----------+
              *                   |
              *                   V
              *           +----------------+
-             *           | m "mmm" R pr88 | 6: a master commit
+             *           | m "mmm" R pr88 | 5: a master commit
              *           +----------------+
              *
              *             +-----------+
@@ -448,7 +460,7 @@ mod tests {
              *                   |
              *                   V
              *           +----------------+
-             *           | m "123" R pr11 | 4: a master commit, high pr number
+             *           | m "123" R pr11 | 3: a master commit, high pr number
              *           +----------------+
              *
              *
@@ -458,12 +470,12 @@ mod tests {
              *                   |
              *                   V
              *           +----------------+
-             *           | m "foo" R pr77 | 5: a master commit
+             *           | m "foo" R pr77 | 4: a master commit
              *           +----------------+
              *                   |
              *                   V
              *           +---------------+
-             *           | t "baz" R pr4 | 7: a try with a low pr, blocked by parent
+             *           | t "baz" R pr4 | 6: a try with a low pr, blocked by parent
              *           +---------------+
              *
              *  The master commits should take priority, then "yee" followed
@@ -476,7 +488,6 @@ mod tests {
                 create_master("123", "345", 11, "days2"),
                 create_try("baz", "foo", 4, "days1"),
                 create_release("v.1.2.3", "days2"),
-                create_try("yee", "rrr", 1, "days2"), // lower PR number takes priority
                 create_try("t1", "rrr", 1, "days1").with_status(BenchmarkRequestStatus::InProgress),
                 create_master("mmm", "aaa", 88, "days2"),
             ];
@@ -492,10 +503,7 @@ mod tests {
 
             let sorted: Vec<BenchmarkRequest> = build_queue(&mut *db, &completed).await.unwrap();
 
-            queue_order_matches(
-                &sorted,
-                &["t1", "v.1.2.3", "yee", "123", "foo", "mmm", "baz"],
-            );
+            queue_order_matches(&sorted, &["t1", "v.1.2.3", "123", "foo", "mmm", "baz"]);
             Ok(ctx)
         })
         .await;