Skip to content

Commit 674ebd1

Browse files
JamesbarfordJamesbarford
committed
Chore; more database error handling and small refactor
1 parent ff57fc5 commit 674ebd1

File tree

6 files changed

+91
-61
lines changed

6 files changed

+91
-61
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: 49 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,10 @@ pub trait Connection: Send + Sync {
182182

183183
/// Add an item to the `benchmark_requests`, if the `benchmark_request`
184184
/// exists it will be ignored
185-
async fn insert_benchmark_request(&self, benchmark_request: &BenchmarkRequest);
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,23 @@ 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;
441-
db.insert_benchmark_request(&release_benchmark_request)
442-
.await;
442+
assert!(db
443+
.insert_benchmark_request(&master_benchmark_request)
444+
.await
445+
.is_ok());
446+
assert!(db
447+
.insert_benchmark_request(&try_benchmark_request)
448+
.await
449+
.is_ok());
450+
assert!(db
451+
.insert_benchmark_request(&release_benchmark_request)
452+
.await
453+
.is_ok());
443454
// duplicate insert
444-
db.insert_benchmark_request(&master_benchmark_request).await;
455+
assert!(db
456+
.insert_benchmark_request(&master_benchmark_request)
457+
.await
458+
.is_err());
445459

446460
Ok(ctx)
447461
})
@@ -484,10 +498,18 @@ mod tests {
484498
);
485499

486500
let db = db.connection().await;
487-
db.insert_benchmark_request(&master_benchmark_request).await;
488-
db.insert_benchmark_request(&try_benchmark_request).await;
489-
db.insert_benchmark_request(&release_benchmark_request)
490-
.await;
501+
assert!(db
502+
.insert_benchmark_request(&master_benchmark_request)
503+
.await
504+
.is_ok());
505+
assert!(db
506+
.insert_benchmark_request(&try_benchmark_request)
507+
.await
508+
.is_ok());
509+
assert!(db
510+
.insert_benchmark_request(&release_benchmark_request)
511+
.await
512+
.is_ok());
491513

492514
let requests = db
493515
.get_benchmark_requests_by_status(&[BenchmarkRequestStatus::ArtifactsReady])
@@ -521,7 +543,10 @@ mod tests {
521543
);
522544

523545
let mut db = db.connection().await;
524-
db.insert_benchmark_request(&master_benchmark_request).await;
546+
assert!(db
547+
.insert_benchmark_request(&master_benchmark_request)
548+
.await
549+
.is_ok());
525550

526551
db.update_benchmark_request_status(
527552
&master_benchmark_request,
@@ -561,7 +586,10 @@ mod tests {
561586
"cranelift",
562587
"",
563588
);
564-
db.insert_benchmark_request(&try_benchmark_request).await;
589+
assert!(db
590+
.insert_benchmark_request(&try_benchmark_request)
591+
.await
592+
.is_ok());
565593
db.attach_shas_to_try_benchmark_request(pr, "foo", "bar")
566594
.await
567595
.unwrap();
@@ -597,7 +625,7 @@ mod tests {
597625
"cranelift",
598626
"",
599627
);
600-
db.insert_benchmark_request(&completed_try).await;
628+
assert!(db.insert_benchmark_request(&completed_try).await.is_ok());
601629

602630
let try_benchmark_request = BenchmarkRequest::create_try(
603631
None,
@@ -609,9 +637,15 @@ mod tests {
609637
"",
610638
);
611639
// deliberately insert twice
612-
db.insert_benchmark_request(&try_benchmark_request).await;
640+
assert!(db
641+
.insert_benchmark_request(&try_benchmark_request)
642+
.await
643+
.is_ok());
613644
// this one should fail
614-
db.insert_benchmark_request(&try_benchmark_request).await;
645+
assert!(db
646+
.insert_benchmark_request(&try_benchmark_request)
647+
.await
648+
.is_err());
615649
db.attach_shas_to_try_benchmark_request(pr, "foo", "bar")
616650
.await
617651
.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)