Skip to content

Commit 2f1d1c2

Browse files
KobzolKobzol
authored
Merge pull request #2198 from Jamesbarford/chore/db-error-handling
Chore; Missed database error handling & small refactor
2 parents ff57fc5 + f955b98 commit 2f1d1c2

File tree

6 files changed

+81
-60
lines changed

6 files changed

+81
-60
lines changed

‎database/src/lib.rs‎

Lines changed: 19 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -805,17 +805,23 @@ pub enum BenchmarkRequestStatus {
805805
Completed,
806806
}
807807

808-
impl fmt::Display for BenchmarkRequestStatus {
809-
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
808+
impl BenchmarkRequestStatus {
809+
pub fn as_str(&self) -> &str {
810810
match self {
811-
BenchmarkRequestStatus::WaitingForArtifacts => write!(f, "waiting_for_artifacts"),
812-
BenchmarkRequestStatus::ArtifactsReady => write!(f, "artifacts_ready"),
813-
BenchmarkRequestStatus::InProgress => write!(f, "in_progress"),
814-
BenchmarkRequestStatus::Completed => write!(f, "completed"),
811+
BenchmarkRequestStatus::WaitingForArtifacts => "waiting_for_artifacts",
812+
BenchmarkRequestStatus::ArtifactsReady => "artifacts_ready",
813+
BenchmarkRequestStatus::InProgress => "in_progress",
814+
BenchmarkRequestStatus::Completed => "completed",
815815
}
816816
}
817817
}
818818

819+
impl fmt::Display for BenchmarkRequestStatus {
820+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
821+
f.write_str(self.as_str())
822+
}
823+
}
824+
819825
impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus {
820826
fn from_sql(
821827
ty: &tokio_postgres::types::Type,
@@ -825,10 +831,10 @@ impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus {
825831
let s: &str = <&str as tokio_postgres::types::FromSql>::from_sql(ty, raw)?;
826832

827833
match s {
828-
"waiting_for_artifacts" => Ok(Self::WaitingForArtifacts),
829-
"artifacts_ready" => Ok(Self::ArtifactsReady),
830-
"in_progress" => Ok(Self::InProgress),
831-
"completed" => Ok(Self::Completed),
834+
x if x == Self::WaitingForArtifacts.as_str() => Ok(Self::WaitingForArtifacts),
835+
x if x == Self::ArtifactsReady.as_str() => Ok(Self::ArtifactsReady),
836+
x if x == Self::InProgress.as_str() => Ok(Self::InProgress),
837+
x if x == Self::Completed.as_str() => Ok(Self::Completed),
832838
other => Err(format!("unknown benchmark_request_status '{other}'").into()),
833839
}
834840
}
@@ -856,30 +862,12 @@ pub enum BenchmarkRequestType {
856862
Release { tag: String },
857863
}
858864

859-
impl BenchmarkRequestType {
860-
pub fn commit_type_str(&self) -> &str {
861-
match self {
862-
BenchmarkRequestType::Try {
863-
sha: _,
864-
parent_sha: _,
865-
pr: _,
866-
} => "try",
867-
BenchmarkRequestType::Master {
868-
sha: _,
869-
parent_sha: _,
870-
pr: _,
871-
} => "master",
872-
BenchmarkRequestType::Release { tag: _ } => "release",
873-
}
874-
}
875-
}
876-
877865
impl fmt::Display for BenchmarkRequestType {
878866
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
879867
match self {
880868
BenchmarkRequestType::Try { .. } => write!(f, "try"),
881869
BenchmarkRequestType::Master { .. } => write!(f, "master"),
882-
BenchmarkRequestType::Release { tag: _ } => write!(f, "release"),
870+
BenchmarkRequestType::Release { .. } => write!(f, "release"),
883871
}
884872
}
885873
}
@@ -975,19 +963,15 @@ impl BenchmarkRequest {
975963
BenchmarkRequestType::Try { pr, .. } | BenchmarkRequestType::Master { pr, .. } => {
976964
Some(pr)
977965
}
978-
BenchmarkRequestType::Release { tag: _ } => None,
966+
BenchmarkRequestType::Release { .. } => None,
979967
}
980968
}
981969

982-
pub fn commit_type(&self) -> &str {
983-
self.commit_type.commit_type_str()
984-
}
985-
986970
pub fn parent_sha(&self) -> Option<&str> {
987971
match &self.commit_type {
988972
BenchmarkRequestType::Try { parent_sha, .. } => parent_sha.as_deref(),
989973
BenchmarkRequestType::Master { parent_sha, .. } => Some(parent_sha),
990-
BenchmarkRequestType::Release { tag: _ } => None,
974+
BenchmarkRequestType::Release { .. } => None,
991975
}
992976
}
993977
}

‎database/src/pool.rs‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,11 @@ pub trait Connection: Send + Sync {
181181
async fn purge_artifact(&self, aid: &ArtifactId);
182182

183183
/// Add an item to the `benchmark_requests`, if the `benchmark_request`
184-
/// exists it will be ignored
185-
async fn insert_benchmark_request(&self, benchmark_request: &BenchmarkRequest);
184+
/// exists an Error will be returned
185+
async fn insert_benchmark_request(
186+
&self,
187+
benchmark_request: &BenchmarkRequest,
188+
) -> anyhow::Result<()>;
186189

187190
/// Gets the benchmark requests matching the status. Optionally provide the
188191
/// number of days from whence to search from
@@ -436,12 +439,20 @@ mod tests {
436439
);
437440

438441
let db = db.connection().await;
439-
db.insert_benchmark_request(&master_benchmark_request).await;
440-
db.insert_benchmark_request(&try_benchmark_request).await;
442+
db.insert_benchmark_request(&master_benchmark_request)
443+
.await
444+
.unwrap();
445+
db.insert_benchmark_request(&try_benchmark_request)
446+
.await
447+
.unwrap();
441448
db.insert_benchmark_request(&release_benchmark_request)
442-
.await;
449+
.await
450+
.unwrap();
443451
// duplicate insert
444-
db.insert_benchmark_request(&master_benchmark_request).await;
452+
assert!(db
453+
.insert_benchmark_request(&master_benchmark_request)
454+
.await
455+
.is_err());
445456

446457
Ok(ctx)
447458
})
@@ -484,10 +495,15 @@ mod tests {
484495
);
485496

486497
let db = db.connection().await;
487-
db.insert_benchmark_request(&master_benchmark_request).await;
488-
db.insert_benchmark_request(&try_benchmark_request).await;
498+
db.insert_benchmark_request(&master_benchmark_request)
499+
.await
500+
.unwrap();
501+
db.insert_benchmark_request(&try_benchmark_request)
502+
.await
503+
.unwrap();
489504
db.insert_benchmark_request(&release_benchmark_request)
490-
.await;
505+
.await
506+
.unwrap();
491507

492508
let requests = db
493509
.get_benchmark_requests_by_status(&[BenchmarkRequestStatus::ArtifactsReady])
@@ -521,7 +537,9 @@ mod tests {
521537
);
522538

523539
let mut db = db.connection().await;
524-
db.insert_benchmark_request(&master_benchmark_request).await;
540+
db.insert_benchmark_request(&master_benchmark_request)
541+
.await
542+
.unwrap();
525543

526544
db.update_benchmark_request_status(
527545
&master_benchmark_request,
@@ -561,7 +579,9 @@ mod tests {
561579
"cranelift",
562580
"",
563581
);
564-
db.insert_benchmark_request(&try_benchmark_request).await;
582+
db.insert_benchmark_request(&try_benchmark_request)
583+
.await
584+
.unwrap();
565585
db.attach_shas_to_try_benchmark_request(pr, "foo", "bar")
566586
.await
567587
.unwrap();
@@ -597,7 +617,7 @@ mod tests {
597617
"cranelift",
598618
"",
599619
);
600-
db.insert_benchmark_request(&completed_try).await;
620+
db.insert_benchmark_request(&completed_try).await.unwrap();
601621

602622
let try_benchmark_request = BenchmarkRequest::create_try(
603623
None,
@@ -609,9 +629,14 @@ mod tests {
609629
"",
610630
);
611631
// deliberately insert twice
612-
db.insert_benchmark_request(&try_benchmark_request).await;
632+
db.insert_benchmark_request(&try_benchmark_request)
633+
.await
634+
.unwrap();
613635
// this one should fail
614-
db.insert_benchmark_request(&try_benchmark_request).await;
636+
assert!(db
637+
.insert_benchmark_request(&try_benchmark_request)
638+
.await
639+
.is_err());
615640
db.attach_shas_to_try_benchmark_request(pr, "foo", "bar")
616641
.await
617642
.unwrap();

‎database/src/pool/postgres.rs‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,7 +1387,10 @@ where
13871387
.unwrap();
13881388
}
13891389

1390-
async fn insert_benchmark_request(&self, benchmark_request: &BenchmarkRequest) {
1390+
async fn insert_benchmark_request(
1391+
&self,
1392+
benchmark_request: &BenchmarkRequest,
1393+
) -> anyhow::Result<()> {
13911394
self.conn()
13921395
.execute(
13931396
r#"
@@ -1401,22 +1404,22 @@ where
14011404
backends,
14021405
profiles
14031406
)
1404-
VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
1405-
ON CONFLICT DO NOTHING;
1407+
VALUES ($1, $2, $3, $4, $5, $6, $7, $8);
14061408
"#,
14071409
&[
14081410
&benchmark_request.tag(),
14091411
&benchmark_request.parent_sha(),
14101412
&benchmark_request.pr().map(|it| *it as i32),
1411-
&benchmark_request.commit_type(),
1412-
&benchmark_request.status.to_string(),
1413+
&benchmark_request.commit_type,
1414+
&benchmark_request.status,
14131415
&benchmark_request.created_at,
14141416
&benchmark_request.backends,
14151417
&benchmark_request.profiles,
14161418
],
14171419
)
14181420
.await
1419-
.unwrap();
1421+
.context("Failed to insert benchmark request")?;
1422+
Ok(())
14201423
}
14211424

14221425
async fn get_benchmark_requests_by_status(

‎database/src/pool/sqlite.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1264,7 +1264,10 @@ impl Connection for SqliteConnection {
12641264
.unwrap();
12651265
}
12661266

1267-
async fn insert_benchmark_request(&self, _benchmark_request: &BenchmarkRequest) {
1267+
async fn insert_benchmark_request(
1268+
&self,
1269+
_benchmark_request: &BenchmarkRequest,
1270+
) -> anyhow::Result<()> {
12681271
no_queue_implementation_abort!()
12691272
}
12701273

‎site/src/job_queue.rs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ async fn create_benchmark_request_master_commits(
4242
"",
4343
"",
4444
);
45-
conn.insert_benchmark_request(&benchmark).await;
45+
if let Err(e) = conn.insert_benchmark_request(&benchmark).await {
46+
log::error!("Failed to insert master benchmark request: {}", e);
47+
}
4648
}
4749
}
4850
Ok(())
@@ -130,7 +132,9 @@ async fn create_benchmark_request_releases(
130132
"",
131133
"",
132134
);
133-
conn.insert_benchmark_request(&release_request).await;
135+
if let Err(e) = conn.insert_benchmark_request(&release_request).await {
136+
log::error!("Failed to insert release benchmark request: {}", e);
137+
}
134138
}
135139
}
136140
Ok(())
@@ -401,7 +405,7 @@ mod tests {
401405
requests: &[BenchmarkRequest],
402406
) {
403407
for request in requests {
404-
conn.insert_benchmark_request(&request).await;
408+
conn.insert_benchmark_request(&request).await.unwrap();
405409
}
406410
}
407411

‎site/src/request_handlers/github.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,9 @@ async fn queue_partial_try_benchmark_request(
9393
backends,
9494
"",
9595
);
96-
conn.insert_benchmark_request(&try_request).await;
96+
if let Err(e) = conn.insert_benchmark_request(&try_request).await {
97+
log::error!("Failed to insert try benchmark request: {}", e);
98+
}
9799
}
98100
}
99101

0 commit comments

Comments
 (0)