PR feedback; improve where clause, improve error message and remove transaction

Author: Jamesbarford

database/src/pool.rs

@@ -201,7 +201,7 @@ pub trait Connection: Send + Sync {
     /// Update a Try commit to have a `sha` and `parent_sha`. Will update the
     /// status of the request too a ready state.
     async fn attach_shas_to_try_benchmark_request(
-        &mut self,
+        &self,
         pr: u32,
         sha: &str,
         parent_sha: &str,
@@ -548,7 +548,7 @@ mod tests {
     async fn updating_try_commits() {
         run_postgres_test(|ctx| async {
             let db = ctx.db_client();
-            let mut db = db.connection().await;
+            let db = db.connection().await;
             let time = chrono::DateTime::from_str("2021-09-01T00:00:00.000Z").unwrap();
             let pr = 42;
 

database/src/pool/postgres.rs

@@ -1527,38 +1527,35 @@ where
     }
 
     async fn attach_shas_to_try_benchmark_request(
-        &mut self,
+        &self,
         pr: u32,
         sha: &str,
         parent_sha: &str,
     ) -> anyhow::Result<()> {
-        let tx = self
-            .conn_mut()
-            .transaction()
-            .await
-            .context("failed to start transaction")?;
-
-        tx.execute(
-            "UPDATE
+        self.conn()
+            .execute(
+                "UPDATE
                 benchmark_request
             SET
                 tag = $1,
                 parent_sha = $2,
                 status = $3
             WHERE
                 pr = $4
-                AND commit_type = 'try';",
-            &[
-                &sha,
-                &parent_sha,
-                &BenchmarkRequestStatus::ArtifactsReady,
-                &(pr as i32),
-            ],
-        )
-        .await
-        .context("failed to execute UPDATE benchmark_request")?;
+                AND commit_type = 'try'
+                AND tag IS NULL
+                AND status = $5;",
+                &[
+                    &sha,
+                    &parent_sha,
+                    &BenchmarkRequestStatus::ArtifactsReady,
+                    &(pr as i32),
+                    &BenchmarkRequestStatus::WaitingForArtifacts,
+                ],
+            )
+            .await
+            .context("failed to execute UPDATE benchmark_request")?;
 
-        tx.commit().await.context("failed to commit transaction")?;
         Ok(())
     }
 }

database/src/pool/sqlite.rs

@@ -1284,7 +1284,7 @@ impl Connection for SqliteConnection {
     }
 
     async fn attach_shas_to_try_benchmark_request(
-        &mut self,
+        &self,
         _pr: u32,
         _sha: &str,
         _parent_sha: &str,

site/src/github.rs

@@ -235,7 +235,7 @@ pub async fn rollup_pr_number(
 }
 
 async fn attach_shas_to_try_benchmark_request(
-    conn: &mut dyn database::pool::Connection,
+    conn: &dyn database::pool::Connection,
     pr: u32,
     sha: &str,
     parent_sha: &str,
@@ -245,7 +245,7 @@ async fn attach_shas_to_try_benchmark_request(
             .attach_shas_to_try_benchmark_request(pr, sha, parent_sha)
             .await
         {
-            log::error!("Cron job failed to execute {}", e);
+            log::error!("Failed to add shas to try commit {}", e);
         }
     }
 }
@@ -274,15 +274,10 @@ pub async fn enqueue_shas(
             sha: commit_response.sha,
             parent_sha: commit_response.parents.remove(0).sha,
         };
-        let mut conn = ctxt.conn().await;
-
-        attach_shas_to_try_benchmark_request(
-            &mut *conn,
-            pr_number,
-            &try_commit.sha,
-            &try_commit.sha,
-        )
-        .await;
+        let conn = ctxt.conn().await;
+
+        attach_shas_to_try_benchmark_request(&*conn, pr_number, &try_commit.sha, &try_commit.sha)
+            .await;
 
         let queued = conn
             .pr_attach_commit(