diff --git a/database/src/lib.rs b/database/src/lib.rs index 0f4c6f1ae..fe9a43810 100644 --- a/database/src/lib.rs +++ b/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, parent_sha: Option, 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, @@ -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, } diff --git a/database/src/pool.rs b/database/src/pool.rs index e71dfffe2..b40085234 100644 --- a/database/src/pool.rs +++ b/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, diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index e0aa9324d..8502f1f91 100644 --- a/database/src/pool/postgres.rs +++ b/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, diff --git a/site/src/job_queue.rs b/site/src/job_queue.rs index ebcd4b37e..c4a5f29b7 100644 --- a/site/src/job_queue.rs +++ b/site/src/job_queue.rs @@ -182,7 +182,16 @@ fn sort_benchmark_requests(done: &HashSet, 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 = 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;