Skip to content
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.

Sign up for GitHub

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

Store assigned PRs in memory #1906

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Store assigned PRs in memory #1906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86a20e2
Store assigned PRs in memory
Kobzol Feb 18, 2025
b7c8cd8
Temporarily remove DB assignment tests
Kobzol Feb 18, 2025
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 src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@ use std::{
};
use tracing as log;

#[derive(Debug, PartialEq, Eq, serde::Deserialize, Clone)]
pub type UserId = u64;

#[derive(Debug, PartialEq, Eq, Hash, serde::Deserialize, Clone)]
pub struct User {
pub login: String,
pub id: u64,
pub id: UserId,
}

impl GithubClient {
Expand Down Expand Up @@ -3048,7 +3050,7 @@ async fn project_items_by_status(
}

/// Retrieve all pull requests in status OPEN that are not drafts
pub async fn retrieve_pull_requests(
pub async fn retrieve_open_pull_requests(
repo: &Repository,
client: &GithubClient,
) -> anyhow::Result<Vec<(User, i32)>> {
Expand Down
4 changes: 4 additions & 0 deletions src/handlers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::config::{self, Config, ConfigurationError};
use crate::github::{Event, GithubClient, IssueCommentAction, IssuesAction, IssuesEvent};
use crate::handlers::pr_tracking::ReviewerWorkqueue;
use octocrab::Octocrab;
use parser::command::{assign::AssignCommand, Command, Input};
use std::fmt;
Expand Down Expand Up @@ -348,4 +349,7 @@ pub struct Context {
pub db: crate::db::ClientPool,
pub username: String,
pub octocrab: Octocrab,
/// Represents the workqueue (assigned open PRs) of individual reviewers.
/// tokio's RwLock is used to avoid deadlocks, since we run on a single-threaded tokio runtime.
pub workqueue: Arc<tokio::sync::RwLock<ReviewerWorkqueue>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I don't quite like storing this state like this - I'd rather have some (Handler, HandlerState) somewhere. But this is fine for now.

Copy link
Contributor Author

@Kobzol Kobzol Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this state has to be available across handlers. It is populated in pull_requests_assignment_update.rs, updated in pr_tracking.rs and it will be read in the future in assign.rs. Thus it's pretty much global state, hence it being stored in Context.

}
99 changes: 36 additions & 63 deletions src/handlers/assign.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
use crate::{
config::{AssignConfig, WarnNonDefaultBranchException},
github::{self, Event, FileDiff, Issue, IssuesAction, Selection},
handlers::{pr_tracking::has_user_capacity, Context, GithubClient, IssuesEvent},
handlers::{Context, GithubClient, IssuesEvent},
interactions::EditIssueBody,
};
use anyhow::{bail, Context as _};
Expand All @@ -39,7 +39,6 @@ use tracing as log;
#[cfg(test)]
mod tests {
mod tests_candidates;
mod tests_db;
mod tests_from_diff;
}

Expand Down Expand Up @@ -560,22 +559,22 @@ pub(super) async fn handle_command(
}
let db_client = ctx.db.get().await;
if is_self_assign(&name, &event.user().login) {
let work_queue = has_user_capacity(&db_client, &name).await;
if work_queue.is_err() {
// NOTE: disabled for now, just log
log::warn!(
"[#{}] PR self-assign failed, DB reported that user {} has no review capacity. Ignoring.",
issue.number,
name
);
// issue
// .post_comment(
// &ctx.github,
// &REVIEWER_HAS_NO_CAPACITY.replace("{username}", &name),
// )
// .await?;
// return Ok(());
}
// let work_queue = has_user_capacity(&db_client, &name).await;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this disabled? We definitely want this logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions that I commented were using the assigned_prs column in the DB, which is no longer used after this PR. So these functions simply do not work anymore (in fact I removed them in this PR).

I'm planning to further work on assigning PR capacity limit, and then refactor the way PRs are assigned, and add back logging (and then we can hopefully enable the new assignment logic again).

I did not want to do all that in this PR, hence I commented this code.

// if work_queue.is_err() {
// // NOTE: disabled for now, just log
// log::warn!(
// "[#{}] PR self-assign failed, DB reported that user {} has no review capacity. Ignoring.",
// issue.number,
// name
// );
// // issue
// // .post_comment(
// // &ctx.github,
// // &REVIEWER_HAS_NO_CAPACITY.replace("{username}", &name),
// // )
// // .await?;
// // return Ok(());
// }

name.to_string()
} else {
Expand Down Expand Up @@ -802,7 +801,7 @@ impl fmt::Display for FindReviewerError {
/// auto-assign groups, or rust-lang team names. It must have at least one
/// entry.
async fn find_reviewer_from_names(
db: &DbClient,
_db: &DbClient,
teams: &Teams,
config: &AssignConfig,
issue: &Issue,
Expand Down Expand Up @@ -841,24 +840,24 @@ async fn find_reviewer_from_names(
}

// filter out team members without capacity
let filtered_candidates = filter_by_capacity(db, &candidates)
.await
.expect("Error while filtering out team members");

if filtered_candidates.is_empty() {
// NOTE: disabled for now, just log
log::info!("[#{}] Filtered list of PR assignee is empty", issue.number);
// return Err(FindReviewerError::AllReviewersFiltered {
// initial: names.to_vec(),
// filtered: names.to_vec(),
// });
}

log::info!(
"[#{}] Filtered list of candidates: {:?}",
issue.number,
filtered_candidates
);
// let filtered_candidates = filter_by_capacity(db, &candidates)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here.

// .await
// .expect("Error while filtering out team members");
//
// if filtered_candidates.is_empty() {
// // NOTE: disabled for now, just log
// log::info!("[#{}] Filtered list of PR assignee is empty", issue.number);
// // return Err(FindReviewerError::AllReviewersFiltered {
// // initial: names.to_vec(),
// // filtered: names.to_vec(),
// // });
// }
//
// log::info!(
// "[#{}] Filtered list of candidates: {:?}",
// issue.number,
// filtered_candidates
// );

// Return unfiltered list of candidates
Ok(candidates
Expand All @@ -868,32 +867,6 @@ async fn find_reviewer_from_names(
.to_string())
}

/// Filter out candidates not having review capacity
async fn filter_by_capacity(
db: &DbClient,
candidates: &HashSet<&str>,
) -> anyhow::Result<HashSet<String>> {
let usernames = candidates
.iter()
.map(|c| *c)
.collect::<Vec<&str>>()
.join(",");

let q = format!(
"
SELECT username
FROM review_prefs r
JOIN users on users.user_id=r.user_id
AND username = ANY('{{ {} }}')
AND CARDINALITY(r.assigned_prs) < LEAST(COALESCE(r.max_assigned_prs,1000000))",
usernames
);
let result = db.query(&q, &[]).await.context("Select DB error")?;
let candidates: HashSet<String> = result.iter().map(|row| row.get("username")).collect();
log::info!("DB returned these candidates: {:?}", candidates);
Ok(candidates)
}

/// Returns a list of candidate usernames (from relevant teams) to choose as a reviewer.
fn candidate_reviewers_from_names<'a>(
teams: &'a Teams,
Expand Down
34 changes: 0 additions & 34 deletions src/handlers/assign/tests/tests_db.rs
View file
Open in desktop

This file was deleted.

Loading