Skip to content

Store try commit dates into the DB properly #1399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions collector/src/api.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
pub mod next_commit {
use database::Commit;

#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
pub struct Commit {
pub sha: String,
pub struct NextCommit {
pub commit: Commit,
pub include: Option<String>,
pub exclude: Option<String>,
pub runs: Option<i32>,
}

#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
pub struct Response {
pub commit: Option<Commit>,
pub commit: Option<NextCommit>,
}
}
7 changes: 3 additions & 4 deletions collector/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1008,12 +1008,11 @@ fn main_result() -> anyhow::Result<i32> {
// no missing commits
return Ok(0);
};
let commit = get_commit_or_fake_it(&next.sha)?;

let pool = database::Pool::open(&db.db);

let sysroot = Sysroot::install(commit.sha.to_string(), &target_triple)
.with_context(|| format!("failed to install sysroot for {:?}", commit))?;
let sysroot = Sysroot::install(next.commit.sha.to_string(), &target_triple)
.with_context(|| format!("failed to install sysroot for {:?}", next.commit))?;

let mut benchmarks = get_benchmarks(
&benchmark_dir,
Expand All @@ -1025,7 +1024,7 @@ fn main_result() -> anyhow::Result<i32> {
let res = bench(
&mut rt,
pool,
&ArtifactId::Commit(commit),
&ArtifactId::Commit(next.commit),
&Profile::all(),
&Scenario::all(),
bench_rustc.bench_rustc,
Expand Down
1 change: 1 addition & 0 deletions database/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub struct QueuedCommit {
pub include: Option<String>,
pub exclude: Option<String>,
pub runs: Option<i32>,
pub commit_date: Option<Date>,
}

#[derive(Debug, Hash, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
Expand Down
8 changes: 7 additions & 1 deletion database/src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,13 @@ pub trait Connection: Send + Sync {
runs: Option<i32>,
);
/// Returns true if this PR was queued waiting for a commit
async fn pr_attach_commit(&self, pr: u32, sha: &str, parent_sha: &str) -> bool;
async fn pr_attach_commit(
&self,
pr: u32,
sha: &str,
parent_sha: &str,
commit_date: Option<DateTime<Utc>>,
) -> bool;
async fn queued_commits(&self) -> Vec<QueuedCommit>;
async fn mark_complete(&self, sha: &str) -> Option<QueuedCommit>;

Expand Down
21 changes: 16 additions & 5 deletions database/src/pool/postgres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,9 @@ static MIGRATIONS: &[&str] = &[
r#"
alter table benchmark add column category text not null DEFAULT 'secondary';
"#,
r#"
alter table pull_request_build add column commit_date timestamptz;
"#,
];

#[async_trait::async_trait]
Expand Down Expand Up @@ -623,12 +626,18 @@ where
log::error!("failed to queue_pr({}, {:?}, {:?}, {:?}): {:?}", pr, include, exclude, runs, e);
}
}
async fn pr_attach_commit(&self, pr: u32, sha: &str, parent_sha: &str) -> bool {
async fn pr_attach_commit(
&self,
pr: u32,
sha: &str,
parent_sha: &str,
commit_date: Option<DateTime<Utc>>,
) -> bool {
self.conn()
.execute(
"update pull_request_build SET bors_sha = $1, parent_sha = $2
"update pull_request_build SET bors_sha = $1, parent_sha = $2, commit_date = $4
where pr = $3 and bors_sha is null",
&[&sha, &parent_sha, &(pr as i32)],
&[&sha, &parent_sha, &(pr as i32), &commit_date],
)
.await
.unwrap()
Expand All @@ -638,7 +647,7 @@ where
let rows = self
.conn()
.query(
"select pr, bors_sha, parent_sha, include, exclude, runs from pull_request_build
"select pr, bors_sha, parent_sha, include, exclude, runs, commit_date from pull_request_build
where complete is false and bors_sha is not null
order by requested asc",
&[],
Expand All @@ -653,6 +662,7 @@ where
include: row.get(3),
exclude: row.get(4),
runs: row.get(5),
commit_date: row.get::<_, Option<_>>(6).map(Date),
})
.collect()
}
Expand All @@ -662,7 +672,7 @@ where
.query_opt(
"update pull_request_build SET complete = true
where bors_sha = $1
returning pr, bors_sha, parent_sha, include, exclude, runs",
returning pr, bors_sha, parent_sha, include, exclude, runs, commit_date",
&[&sha],
)
.await
Expand All @@ -674,6 +684,7 @@ where
include: row.get(3),
exclude: row.get(4),
runs: row.get(5),
commit_date: row.get::<_, Option<_>>(6).map(Date),
})
}
async fn collection_id(&self, version: &str) -> CollectionId {
Expand Down
22 changes: 16 additions & 6 deletions database/src/pool/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
ArtifactId, Benchmark, BenchmarkData, CollectionId, Commit, CommitType, Date, Profile,
};
use crate::{ArtifactIdNumber, Index, QueryDatum, QueuedCommit};
use chrono::{DateTime, TimeZone, Utc};
use chrono::{DateTime, NaiveDateTime, TimeZone, Utc};
use hashbrown::HashMap;
use rusqlite::params;
use rusqlite::OptionalExtension;
Expand Down Expand Up @@ -321,6 +321,7 @@ static MIGRATIONS: &[Migration] = &[
"#,
),
Migration::new("alter table benchmark add column category text not null default ''"),
Migration::new("alter table pull_request_build add column commit_date timestamp"),
];

#[async_trait::async_trait]
Expand Down Expand Up @@ -564,21 +565,28 @@ impl Connection for SqliteConnection {
.execute(params![pr, include, exclude, &runs])
.unwrap();
}
async fn pr_attach_commit(&self, pr: u32, sha: &str, parent_sha: &str) -> bool {
async fn pr_attach_commit(
&self,
pr: u32,
sha: &str,
parent_sha: &str,
commit_date: Option<DateTime<Utc>>,
) -> bool {
let timestamp = commit_date.map(|d| d.timestamp());
self.raw_ref()
.prepare_cached(
"update pull_request_build SET bors_sha = ?, parent_sha = ?
"update pull_request_build SET bors_sha = ?, parent_sha = ?, commit_date = ?
where pr = ? and bors_sha is null",
)
.unwrap()
.execute(params![sha, parent_sha, pr])
.execute(params![sha, parent_sha, timestamp, pr])
.unwrap()
> 0
}
async fn queued_commits(&self) -> Vec<QueuedCommit> {
self.raw_ref()
.prepare_cached(
"select pr, bors_sha, parent_sha, include, exclude, runs from pull_request_build
"select pr, bors_sha, parent_sha, include, exclude, runs, commit_date from pull_request_build
where complete is false and bors_sha is not null
order by requested asc",
)
Expand All @@ -593,6 +601,7 @@ impl Connection for SqliteConnection {
include: row.get(3).unwrap(),
exclude: row.get(4).unwrap(),
runs: row.get(5).unwrap(),
commit_date: row.get::<_, Option<i64>>(6).unwrap().map(|timestamp| Date(DateTime::from_utc(NaiveDateTime::from_timestamp(timestamp, 0), Utc)))
})
})
.collect::<Result<Vec<_>, _>>()
Expand All @@ -612,7 +621,7 @@ impl Connection for SqliteConnection {
assert_eq!(count, 1, "sha is unique column");
self.raw_ref()
.query_row(
"select pr, sha, parent_sha, include, exclude, runs from pull_request_build
"select pr, sha, parent_sha, include, exclude, runs, commit_date from pull_request_build
where sha = ?",
params![sha],
|row| {
Expand All @@ -623,6 +632,7 @@ impl Connection for SqliteConnection {
include: row.get(3).unwrap(),
exclude: row.get(4).unwrap(),
runs: row.get(5).unwrap(),
commit_date: row.get::<_, Option<i64>>(6).unwrap().map(|timestamp| Date(DateTime::from_utc(NaiveDateTime::from_timestamp(timestamp, 0), Utc)))
})
},
)
Expand Down
9 changes: 7 additions & 2 deletions site/src/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,13 @@ pub async fn enqueue_shas(
};
let queued = {
let conn = ctxt.conn().await;
conn.pr_attach_commit(pr_number, &try_commit.sha, &try_commit.parent_sha)
.await
conn.pr_attach_commit(
pr_number,
&try_commit.sha,
&try_commit.parent_sha,
Some(commit_response.commit.committer.date),
)
.await
};
if queued {
if !msg.is_empty() {
Expand Down
7 changes: 7 additions & 0 deletions site/src/github/client.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use anyhow::Context;
use chrono::{DateTime, Utc};
use http::header::USER_AGENT;

use crate::{api::github::Issue, load::SiteCtxt};
Expand Down Expand Up @@ -253,6 +254,12 @@ pub struct InnerCommit {
#[serde(default)]
pub message: String,
pub tree: CommitTree,
pub committer: Committer,
}

#[derive(Debug, Clone, serde::Deserialize)]
pub struct Committer {
pub date: DateTime<Utc>,
}

#[derive(Debug, Clone, serde::Deserialize)]
Expand Down
8 changes: 7 additions & 1 deletion site/src/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,7 @@ fn calculate_missing_from(
include,
exclude,
runs,
commit_date,
} in queued_pr_commits
.into_iter()
// filter out any queued PR master commits (leaving only try commits)
Expand All @@ -322,7 +323,7 @@ fn calculate_missing_from(
queue.push((
Commit {
sha: sha.to_string(),
date: Date::ymd_hms(2001, 01, 01, 0, 0, 0),
date: commit_date.unwrap_or(Date::empty()),
r#type: CommitType::Try,
},
MissingReason::Try {
Expand Down Expand Up @@ -499,6 +500,7 @@ mod tests {
include: None,
exclude: None,
runs: None,
commit_date: None,
},
QueuedCommit {
sha: "b".into(),
Expand All @@ -507,6 +509,7 @@ mod tests {
include: None,
exclude: None,
runs: None,
commit_date: None,
},
QueuedCommit {
sha: "a".into(),
Expand All @@ -515,6 +518,7 @@ mod tests {
include: None,
exclude: None,
runs: None,
commit_date: None,
},
];
let in_progress_artifacts = vec![];
Expand Down Expand Up @@ -606,6 +610,7 @@ mod tests {
include: None,
exclude: None,
runs: None,
commit_date: None,
},
// A try run
QueuedCommit {
Expand All @@ -615,6 +620,7 @@ mod tests {
include: None,
exclude: None,
runs: None,
commit_date: None,
},
];
let in_progress_artifacts = vec![];
Expand Down
9 changes: 6 additions & 3 deletions site/src/request_handlers/next_commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ pub async fn handle_next_commit(ctxt: Arc<SiteCtxt>) -> next_commit::Response {
// TODO: add capability of doing the following in one step
// to avoid possibile illegal inbetween states.
conn.queue_pr(pr, None, None, None).await;
if !conn.pr_attach_commit(pr, &commit.sha, parent_sha).await {
if !conn
.pr_attach_commit(pr, &commit.sha, parent_sha, None)
.await
{
log::error!("failed to attach commit {} to PR queue", commit.sha);
}
}
Expand Down Expand Up @@ -49,8 +52,8 @@ pub async fn handle_next_commit(ctxt: Arc<SiteCtxt>) -> next_commit::Response {
commit.sha,
missing_reason_dbg
);
Some(next_commit::Commit {
sha: commit.sha,
Some(next_commit::NextCommit {
commit,
include,
exclude,
runs,
Expand Down