-
Notifications
You must be signed in to change notification settings - Fork 41
feat(rollups): add rollup_contents mapping #519
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| /.sqlx/*.json binary linguist-generated=true | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| DROP TABLE IF EXISTS rollup_contents; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,11 @@ | ||||||
| CREATE TABLE IF NOT EXISTS rollup_contents | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the member terminology that you have used, so let's go with it.
Suggested change
|
||||||
| ( | ||||||
| -- We have to store the PR through their numbers rather than a FK to the pull_request table. This is due to the rollup PR | ||||||
| -- being created at the time of record insertion in this table, so we won't have an entry in pull_request for it | ||||||
| rollup_pr_number BIGINT NOT NULL, | ||||||
| member_pr_number BIGINT NOT NULL, | ||||||
| repository TEXT NOT NULL, | ||||||
|
|
||||||
| PRIMARY KEY (repository, rollup_pr_number, member_pr_number) | ||||||
| ); | ||||||
| CREATE INDEX rollup_contents_member_pr_number_idx ON rollup_contents (repository, member_pr_number); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per my other comment, I think that it makes more sense to create the index on the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will proceed removing this index, then. For the index on the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, also including repository. Fair enough, as long as the index is used for the query I posted, it should be fine. I ran some simulations with 20k PRs and 3k rollup_member rows on a local Postgres instance, and |
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,10 @@ use sqlx::PgPool; | |||||
|
|
||||||
| use crate::bors::comment::CommentTag; | ||||||
| use crate::bors::{BuildKind, PullRequestStatus, RollupMode}; | ||||||
| use crate::database::operations::{get_merge_queue_prs, update_pr_auto_build_id}; | ||||||
| use crate::database::operations::{ | ||||||
| create_rollup_pr_content, get_merge_queue_prs, get_rollup_pr_contents, get_rollups_for_pr, | ||||||
| update_pr_auto_build_id, | ||||||
| }; | ||||||
| use crate::database::{ | ||||||
| BuildModel, BuildStatus, CommentModel, PullRequestModel, RepoModel, TreeState, WorkflowModel, | ||||||
| WorkflowStatus, WorkflowType, | ||||||
|
|
@@ -344,4 +347,39 @@ impl PgDbClient { | |||||
| pub async fn delete_tagged_bot_comment(&self, comment: &CommentModel) -> anyhow::Result<()> { | ||||||
| delete_tagged_bot_comment(&self.pool, comment.id).await | ||||||
| } | ||||||
|
|
||||||
| /// Register the contents of a rollup in the DB. | ||||||
| /// The contents are stored as rollup PR number - member PR number records, scoped under a specific repository. | ||||||
| /// All records are inserted in a single transaction. | ||||||
| pub async fn create_rollup( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think that's a bit less misleading. |
||||||
| &self, | ||||||
| repo: &GithubRepoName, | ||||||
| rollup_pr_number: &PullRequestNumber, | ||||||
| member_pr_numbers: &[PullRequestNumber], | ||||||
| ) -> anyhow::Result<()> { | ||||||
| let mut tx = self.pool.begin().await?; | ||||||
| for member_pr_number in member_pr_numbers { | ||||||
| create_rollup_pr_content(&mut *tx, repo, rollup_pr_number, member_pr_number).await?; | ||||||
| } | ||||||
| tx.commit().await?; | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| /// Returns the numbers of all the PRs associated with the requested rollup. | ||||||
| pub async fn get_rollup_pr_contents( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the main operation that we will want to do is load members information for a set of PRs that will be displayed on the queue page. So I'm thinking something like:
And then in the frontend, we'll build a reverse index, so that we can also associate a PR with its rollup(s). As an alternative to 1) + 2), we could directly attach a bool whether a given PR is a rollup or not to the return value of 1), and then only ask for members from actual rollups, to reduce the DB traffic slightly. But we'd need to take a look at the query plans to see if it makes sense. And probably perf. will be fine with both approaches. |
||||||
| &self, | ||||||
| repo: &GithubRepoName, | ||||||
| rollup_pr_number: &PullRequestNumber, | ||||||
| ) -> anyhow::Result<Vec<PullRequestNumber>> { | ||||||
| get_rollup_pr_contents(&self.pool, repo, rollup_pr_number).await | ||||||
| } | ||||||
|
|
||||||
| /// Returns all the rollups associated with the requested PR. | ||||||
| pub async fn get_rollups_for_pr( | ||||||
| &self, | ||||||
| repo: &GithubRepoName, | ||||||
| pr_number: &PullRequestNumber, | ||||||
| ) -> anyhow::Result<Vec<PullRequestNumber>> { | ||||||
| get_rollups_for_pr(&self.pool, repo, pr_number).await | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have any specific use-case for this? GitHub already marked them as generated before.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes, it's mostly for the
binarypart, I have a windows workstation and the sqlx command to generate those files was causing diffs due to line endings. I copied this file from the docs.rs repo to avoid this issue. I can remove it from the PR as it's out of scope, if you prefer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Let's keep it then, no real harm in that.