diff --git a/database/src/lib.rs b/database/src/lib.rs index 5d0c142e1..f2084fe18 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -872,7 +872,7 @@ pub enum BenchmarkRequestType { /// A Master commit Master { sha: String, - parent_sha: String, + parent_sha: Option, pr: u32, }, /// A release only has a tag @@ -932,12 +932,17 @@ impl BenchmarkRequest { } /// Create a master benchmark request that is in the `ArtifactsReady` status. - pub fn create_master(sha: &str, parent_sha: &str, pr: u32, commit_date: DateTime) -> Self { + pub fn create_master( + sha: &str, + parent_sha: Option<&str>, + pr: u32, + commit_date: DateTime, + ) -> Self { Self { commit_type: BenchmarkRequestType::Master { pr, sha: sha.to_string(), - parent_sha: parent_sha.to_string(), + parent_sha: parent_sha.map(|it| it.to_string()), }, commit_date: Some(commit_date), created_at: Utc::now(), @@ -968,8 +973,8 @@ impl BenchmarkRequest { pub fn parent_sha(&self) -> Option<&str> { match &self.commit_type { - BenchmarkRequestType::Try { parent_sha, .. } => parent_sha.as_deref(), - BenchmarkRequestType::Master { parent_sha, .. } => Some(parent_sha), + BenchmarkRequestType::Try { parent_sha, .. } + | BenchmarkRequestType::Master { parent_sha, .. } => parent_sha.as_deref(), BenchmarkRequestType::Release { .. } => None, } } @@ -1036,7 +1041,7 @@ impl BenchmarkRequest { } pub fn is_in_progress(&self) -> bool { - matches!(self.status, BenchmarkRequestStatus::InProgress { .. }) + matches!(self.status, BenchmarkRequestStatus::InProgress) } } diff --git a/database/src/pool.rs b/database/src/pool.rs index 7a6cac627..12101af28 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -505,7 +505,7 @@ mod tests { db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-1", - "parent-sha-1", + Some("parent-sha-1"), 42, Utc::now(), )) @@ -559,7 +559,7 @@ mod tests { db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-1", - "parent-sha-1", + Some("parent-sha-1"), 42, Utc::now(), )) @@ -568,7 +568,7 @@ mod tests { db.insert_benchmark_request(&BenchmarkRequest::create_master( "a-sha-2", - "parent-sha-2", + Some("parent-sha-2"), 42, Utc::now(), )) @@ -585,23 +585,26 @@ mod tests { run_postgres_test(|ctx| async { let db = ctx.db(); + ctx.insert_master_request("parent-sha-1", None, 68).await; + ctx.insert_master_request("parent-sha-2", None, 419).await; + // ArtifactsReady - let req_a = ctx.insert_master_request("sha-1", "parent-sha-1", 42).await; + let req_a = ctx + .insert_master_request("sha-1", Some("parent-sha-1"), 42) + .await; // ArtifactsReady let req_b = ctx.insert_release_request("1.80.0").await; // WaitingForArtifacts ctx.insert_try_request(50).await; // InProgress - let req_d = ctx.insert_master_request("sha-2", "parent-sha-2", 51).await; + let req_d = ctx + .insert_master_request("sha-2", Some("parent-sha-2"), 51) + .await; // Completed ctx.insert_release_request("1.79.0").await; ctx.complete_request("1.79.0").await; - ctx.insert_master_request("parent-sha-1", "grandparent-sha-0", 100) - .await; ctx.complete_request("parent-sha-1").await; - ctx.insert_master_request("parent-sha-2", "grandparent-sha-1", 101) - .await; ctx.complete_request("parent-sha-2").await; db.update_benchmark_request_status("sha-2", BenchmarkRequestStatus::InProgress) @@ -678,7 +681,7 @@ mod tests { let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap(); let benchmark_request = - BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + BenchmarkRequest::create_master("sha-1", Some("parent-sha-1"), 42, time); // Insert the request so we don't violate the foreign key db.insert_benchmark_request(&benchmark_request) @@ -833,7 +836,7 @@ mod tests { .unwrap(); let benchmark_request = - BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + BenchmarkRequest::create_master("sha-1", Some("parent-sha-1"), 42, time); // Insert the request so we don't violate the foreign key db.insert_benchmark_request(&benchmark_request) @@ -902,7 +905,7 @@ mod tests { assert!(insert_result.is_ok()); let benchmark_request = - BenchmarkRequest::create_master("sha-1", "parent-sha-1", 42, time); + BenchmarkRequest::create_master("sha-1", Some("parent-sha-1"), 42, time); db.insert_benchmark_request(&benchmark_request) .await .unwrap(); @@ -1045,7 +1048,7 @@ mod tests { RequestBuilder::master( db, &format!("sha{}", id), - &format!("sha{}", id - 1), + Some(&format!("sha{}", id - 1)), id, ) .await @@ -1057,7 +1060,7 @@ mod tests { } // Create an additional non-completed request - ctx.insert_master_request("foo", "bar", 1000).await; + ctx.insert_master_request("foo", Some("bar"), 1000).await; // Request 1 will have artifact with errors let aid1 = ctx.upsert_master_artifact("sha1").await; @@ -1112,10 +1115,10 @@ mod tests { let collector = ctx.add_collector(Default::default()).await; // Artifacts ready request, should be ignored - RequestBuilder::master(db, "foo", "bar", 1000).await; + RequestBuilder::master(db, "foo", Some("bar"), 1000).await; // Create a completed parent with jobs - let completed = RequestBuilder::master(db, "sha4-parent", "sha0", 1001) + let completed = RequestBuilder::master(db, "sha4-parent", Some("sha0"), 1001) .await .add_jobs( db, @@ -1126,13 +1129,13 @@ mod tests { .await; // In progress request without a parent - let req1 = RequestBuilder::master(db, "sha1", "sha0", 1) + let req1 = RequestBuilder::master(db, "sha1", Some("sha0"), 1) .await .set_in_progress(db) .await; // In progress request with a parent that has no jobs - let req2 = RequestBuilder::master(db, "sha2", "sha1", 2) + let req2 = RequestBuilder::master(db, "sha2", Some("sha1"), 2) .await .add_jobs( db, @@ -1143,7 +1146,7 @@ mod tests { .await; // In progress request with a parent that has jobs - let req3 = RequestBuilder::master(db, "sha3", "sha2", 3) + let req3 = RequestBuilder::master(db, "sha3", Some("sha2"), 3) .await .add_jobs( db, @@ -1154,7 +1157,7 @@ mod tests { .await; // In progress request with a parent that has jobs, but is completed - let req4 = RequestBuilder::master(db, "sha4", completed.tag(), 4) + let req4 = RequestBuilder::master(db, "sha4", Some(completed.tag()), 4) .await .add_jobs( db, diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 8fa0988eb..be0dcb28b 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1597,6 +1597,15 @@ where &self, benchmark_request: &BenchmarkRequest, ) -> anyhow::Result<()> { + // We sometimes mark the `parent_sha` as NULL if the `parent_sha` does + // not exist as a `benchmark_request`. This can occur when we start the + // system and previous commits do not exist in the database. + // + // We thought about doing this at the point of job creation however + // that would then end up swallowing some possibly errors when + // inserting. Namely a job should not be created if a tag does not + // exist in the `benchmark_request` table. Thus it's living here for + // the time being. self.conn() .execute( r#" @@ -1611,8 +1620,21 @@ where profiles, commit_date ) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9); - "#, + VALUES ( + $1, + CASE + WHEN EXISTS (SELECT 1 FROM benchmark_request WHERE tag = $2) + THEN $2 + ELSE NULL + END, + $3, + $4, + $5, + $6, + $7, + $8, + $9 + );"#, &[ &benchmark_request.tag(), &benchmark_request.parent_sha(), @@ -2290,7 +2312,7 @@ fn row_to_benchmark_request(row: &Row, row_offset: Option) -> BenchmarkRe BENCHMARK_REQUEST_MASTER_STR => BenchmarkRequest { commit_type: BenchmarkRequestType::Master { sha: tag.expect("Master commit in the DB without a SHA"), - parent_sha: parent_sha.expect("Master commit in the DB without a parent SHA"), + parent_sha, pr: pr.expect("Master commit in the DB without a PR"), }, commit_date, diff --git a/database/src/tests/builder.rs b/database/src/tests/builder.rs index 48679e657..7daba4f9f 100644 --- a/database/src/tests/builder.rs +++ b/database/src/tests/builder.rs @@ -11,7 +11,7 @@ pub struct RequestBuilder { } impl RequestBuilder { - pub async fn master(db: &dyn Connection, tag: &str, parent: &str, pr: u32) -> Self { + pub async fn master(db: &dyn Connection, tag: &str, parent: Option<&str>, pr: u32) -> Self { let request = BenchmarkRequest::create_master(tag, parent, pr, Utc::now()); db.insert_benchmark_request(&request).await.unwrap(); Self { diff --git a/database/src/tests/mod.rs b/database/src/tests/mod.rs index 9d88a60ce..dff4880b6 100644 --- a/database/src/tests/mod.rs +++ b/database/src/tests/mod.rs @@ -131,7 +131,7 @@ impl TestContext { pub async fn insert_master_request( &self, sha: &str, - parent: &str, + parent: Option<&str>, pr: u32, ) -> BenchmarkRequest { let req = BenchmarkRequest::create_master(sha, parent, pr, Utc::now()); diff --git a/site/src/job_queue/mod.rs b/site/src/job_queue/mod.rs index 6c23783dd..5d9dcfbf0 100644 --- a/site/src/job_queue/mod.rs +++ b/site/src/job_queue/mod.rs @@ -47,7 +47,7 @@ async fn create_benchmark_request_master_commits( let pr = master_commit.pr.unwrap_or(0); let benchmark = BenchmarkRequest::create_master( &master_commit.sha, - &master_commit.parent_sha, + Some(&master_commit.parent_sha), pr, master_commit.time, ); @@ -334,9 +334,7 @@ async fn cron_enqueue_jobs(ctxt: &SiteCtxt) -> anyhow::Result<()> { false, *pr, sha.clone().expect("Completed try commit without a SHA"), - parent_sha - .clone() - .expect("Completed try commit without a parent SHA"), + parent_sha.clone(), ), BenchmarkRequestType::Master { pr, @@ -345,24 +343,27 @@ async fn cron_enqueue_jobs(ctxt: &SiteCtxt) -> anyhow::Result<()> { } => (true, *pr, sha.clone(), parent_sha.clone()), BenchmarkRequestType::Release { .. } => continue, }; - let commit = QueuedCommit { - pr, - sha, - parent_sha, - include: None, - exclude: None, - runs: None, - commit_date: request.commit_date().map(Date), - backends: Some( - request - .backends()? - .into_iter() - .map(|b| b.as_str()) - .collect::>() - .join(","), - ), - }; - post_comparison_comment(ctxt, commit, is_master).await?; + + if let Some(parent_sha) = parent_sha { + let commit = QueuedCommit { + pr, + sha, + parent_sha, + include: None, + exclude: None, + runs: None, + commit_date: request.commit_date().map(Date), + backends: Some( + request + .backends()? + .into_iter() + .map(|b| b.as_str()) + .collect::>() + .join(","), + ), + }; + post_comparison_comment(ctxt, commit, is_master).await?; + } } } @@ -401,7 +402,7 @@ mod tests { CodegenBackend, Profile, Target, }; - fn create_master(sha: &str, parent: &str, pr: u32) -> BenchmarkRequest { + fn create_master(sha: &str, parent: Option<&str>, pr: u32) -> BenchmarkRequest { BenchmarkRequest::create_master(sha, parent, pr, Utc::now()) } @@ -546,16 +547,16 @@ mod tests { * +----------------+ **/ let requests = vec![ - create_master("bar", "parent1", 10), - create_master("345", "parent2", 11), - create_master("aaa", "parent3", 12), - create_master("rrr", "parent4", 13), - create_master("123", "bar", 14), - create_master("foo", "345", 15), + create_master("bar", Some("parent1"), 10), + create_master("345", Some("parent2"), 11), + create_master("aaa", Some("parent3"), 12), + create_master("rrr", Some("parent4"), 13), + create_master("123", Some("bar"), 14), + create_master("foo", Some("345"), 15), create_try(16), create_release("v1.2.3"), create_try(17), - create_master("mmm", "aaa", 18), + create_master("mmm", Some("aaa"), 18), ]; db_insert_requests(db, &requests).await;