From 75c879645873250e53ba2399bf5a15fd1d94b7db Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 9 Apr 2021 11:02:59 +0200 Subject: Simplify implementation of subcommand "db jobs" This patch simplifies the implementation by using the `.into_boxed()` function from the diesel query builder and appling the filters from the CLI one-by-one. Signed-off-by: Matthias Beyer --- src/commands/db.rs | 90 +++++++++++++++++++----------------------------------- 1 file changed, 31 insertions(+), 59 deletions(-) diff --git a/src/commands/db.rs b/src/commands/db.rs index 0f11837..8b51570 100644 --- a/src/commands/db.rs +++ b/src/commands/db.rs @@ -290,8 +290,6 @@ fn submits(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { } fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { - use crate::schema::jobs::dsl; - let csv = matches.is_present("csv"); let hdrs = crate::commands::util::mk_header(vec![ "id", @@ -310,67 +308,41 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { .map(crate::util::env::parse_to_env) .transpose()?; - let jobs = matches + let sel = schema::jobs::table + .inner_join(schema::submits::table) + .inner_join(schema::endpoints::table) + .inner_join(schema::packages::table) + .left_outer_join(schema::job_envs::table.inner_join(schema::envvars::table)) + .into_boxed(); + + let sel = if let Some(submit_uuid) = matches .value_of("submit_uuid") .map(uuid::Uuid::parse_str) .transpose()? - .map(|submit_uuid| { - let sel = schema::jobs::table - .inner_join(schema::submits::table) - .inner_join(schema::endpoints::table) - .inner_join(schema::packages::table) - .left_outer_join(schema::job_envs::table.inner_join(schema::envvars::table)) - .filter(schema::submits::uuid.eq(&submit_uuid)) - .into_boxed(); - - let sel = if let Some((name, val)) = env_filter_tpl.as_ref() { - use crate::diesel::BoolExpressionMethods; - - sel.filter({ - schema::envvars::dsl::name.eq(name.as_ref()) - .and(schema::envvars::dsl::value.eq(val)) - }) - } else { - sel - }; - - sel.load::<( - models::Job, - models::Submit, - models::Endpoint, - models::Package, - Option<(models::JobEnv, models::EnvVar)>, - )>(&conn) - .map_err(Error::from) + { + sel.filter(schema::submits::uuid.eq(submit_uuid)) + } else { + sel + }; + + let sel = if let Some((name, val)) = env_filter_tpl.as_ref() { + use crate::diesel::BoolExpressionMethods; + + sel.filter({ + schema::envvars::dsl::name.eq(name.as_ref()) + .and(schema::envvars::dsl::value.eq(val)) }) - .unwrap_or_else(|| { - let sel = dsl::jobs - .inner_join(crate::schema::submits::table) - .inner_join(crate::schema::endpoints::table) - .inner_join(crate::schema::packages::table) - .left_outer_join(schema::job_envs::table.inner_join(schema::envvars::table)) - .into_boxed(); - - let sel = if let Some((name, val)) = env_filter_tpl.as_ref() { - use crate::diesel::BoolExpressionMethods; - - sel.filter({ - schema::envvars::dsl::name.eq(name.as_ref()) - .and(schema::envvars::dsl::value.eq(val)) - }) - } else { - sel - }; - - sel.load::<( - models::Job, - models::Submit, - models::Endpoint, - models::Package, - Option<(models::JobEnv, models::EnvVar)>, - )>(&conn) - .map_err(Error::from) - })?; + } else { + sel + }; + + let jobs = sel.load::<( + models::Job, + models::Submit, + models::Endpoint, + models::Package, + Option<(models::JobEnv, models::EnvVar)>, + )>(&conn)?; let data = jobs .iter() -- cgit v1.2.3 From 82121de9dff59ed0b18096d5416d7e6738ddf2ca Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 9 Apr 2021 11:18:24 +0200 Subject: Rewrite jobs-env-filtering This patch rewrites the filtering for environment variables in the "db jobs" subcommand. The output does not contain the environment variables anymore, which is a drawback, but the implementation is _way_ more easy to understand, which I consider an bigger advantage. The implementation now actually queries the database twice: first it gets all job ids from the JobEnv->EnvVar mapping and then builds a filter based on these IDs for the job output. Signed-off-by: Matthias Beyer --- src/commands/db.rs | 58 ++++++++++++++++++++---------------------------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/src/commands/db.rs b/src/commands/db.rs index 8b51570..d8669aa 100644 --- a/src/commands/db.rs +++ b/src/commands/db.rs @@ -300,19 +300,13 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { "success", "package", "version", - "Env", ]); let conn = crate::db::establish_connection(conn_cfg)?; - let env_filter_tpl = matches - .value_of("filter_env") - .map(crate::util::env::parse_to_env) - .transpose()?; let sel = schema::jobs::table .inner_join(schema::submits::table) .inner_join(schema::endpoints::table) .inner_join(schema::packages::table) - .left_outer_join(schema::job_envs::table.inner_join(schema::envvars::table)) .into_boxed(); let sel = if let Some(submit_uuid) = matches @@ -325,39 +319,30 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { sel }; - let sel = if let Some((name, val)) = env_filter_tpl.as_ref() { - use crate::diesel::BoolExpressionMethods; + // Filter for environment variables from the CLI + // + // If we get a filter for environment on CLI, we fetch all job ids that are associated with the + // passed environment variables and make `sel` filter for those. + let sel = if let Some((name, val)) = matches.value_of("filter_env").map(crate::util::env::parse_to_env).transpose()? { + let jids = schema::envvars::table + .filter({ + use crate::diesel::BoolExpressionMethods; + schema::envvars::dsl::name.eq(name.as_ref()) + .and(schema::envvars::dsl::value.eq(val)) + }) + .inner_join(schema::job_envs::table) + .select(schema::job_envs::all_columns) + .load::(&conn) + .map(|jobenvs| jobenvs.into_iter().map(|je| je.job_id).collect::>())?; - sel.filter({ - schema::envvars::dsl::name.eq(name.as_ref()) - .and(schema::envvars::dsl::value.eq(val)) - }) + sel.filter(schema::jobs::dsl::id.eq_any(jids)) } else { sel }; - let jobs = sel.load::<( - models::Job, - models::Submit, - models::Endpoint, - models::Package, - Option<(models::JobEnv, models::EnvVar)>, - )>(&conn)?; - - let data = jobs - .iter() - .group_by(|(job, submit, ep, package, _)| { - (job, submit, ep, package) - }); - - let data = data + let data = sel.load::<(models::Job, models::Submit, models::Endpoint, models::Package)>(&conn)? .into_iter() - .map(|((job, submit, ep, package), grouped)| { - let envs = grouped - .filter_map(|opt| opt.4.as_ref()) - .map(|tpl| format!("{}={}", tpl.1.name, tpl.1.value)) - .join(", "); - + .map(|(job, submit, ep, package)| { let success = crate::log::ParsedLog::build_from(&job.log_text)? .is_successfull() .map(|b| if b { "yes" } else { "no" }) @@ -369,11 +354,10 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { submit.uuid.to_string(), job.uuid.to_string(), submit.submit_time.to_string(), - ep.name.clone(), + ep.name, success, - package.name.clone(), - package.version.clone(), - envs, + package.name, + package.version, ]) }) .collect::>>()?; -- cgit v1.2.3 From 7e84da5fef77ffaa1b7462b7eb647e48d8c5613c Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 9 Apr 2021 11:32:35 +0200 Subject: Optimize: Only load the job ids from the DB in the first place Signed-off-by: Matthias Beyer --- src/commands/db.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/commands/db.rs b/src/commands/db.rs index d8669aa..a8d8b3c 100644 --- a/src/commands/db.rs +++ b/src/commands/db.rs @@ -331,9 +331,8 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { .and(schema::envvars::dsl::value.eq(val)) }) .inner_join(schema::job_envs::table) - .select(schema::job_envs::all_columns) - .load::(&conn) - .map(|jobenvs| jobenvs.into_iter().map(|je| je.job_id).collect::>())?; + .select(schema::job_envs::job_id) + .load::(&conn)?; sel.filter(schema::jobs::dsl::id.eq_any(jids)) } else { -- cgit v1.2.3 From 56a0036cb55fad022c05b961563b46d1c284e02d Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 9 Apr 2021 11:33:07 +0200 Subject: Fix: clap value querying should match CLI spec Signed-off-by: Matthias Beyer --- src/commands/db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/db.rs b/src/commands/db.rs index a8d8b3c..5961c77 100644 --- a/src/commands/db.rs +++ b/src/commands/db.rs @@ -323,7 +323,7 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { // // If we get a filter for environment on CLI, we fetch all job ids that are associated with the // passed environment variables and make `sel` filter for those. - let sel = if let Some((name, val)) = matches.value_of("filter_env").map(crate::util::env::parse_to_env).transpose()? { + let sel = if let Some((name, val)) = matches.value_of("env_filter").map(crate::util::env::parse_to_env).transpose()? { let jids = schema::envvars::table .filter({ use crate::diesel::BoolExpressionMethods; -- cgit v1.2.3 From 66e8826465cc51ceb723223a6ec567ee5543a20b Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 9 Apr 2021 11:33:14 +0200 Subject: Add debug output Signed-off-by: Matthias Beyer --- src/commands/db.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/commands/db.rs b/src/commands/db.rs index 5961c77..7dbf08a 100644 --- a/src/commands/db.rs +++ b/src/commands/db.rs @@ -24,6 +24,7 @@ use diesel::JoinOnDsl; use diesel::QueryDsl; use diesel::RunQueryDsl; use itertools::Itertools; +use log::debug; use log::info; use crate::config::Configuration; @@ -324,6 +325,7 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { // If we get a filter for environment on CLI, we fetch all job ids that are associated with the // passed environment variables and make `sel` filter for those. let sel = if let Some((name, val)) = matches.value_of("env_filter").map(crate::util::env::parse_to_env).transpose()? { + debug!("Filtering for ENV: {} = {}", name, val); let jids = schema::envvars::table .filter({ use crate::diesel::BoolExpressionMethods; @@ -334,6 +336,7 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { .select(schema::job_envs::job_id) .load::(&conn)?; + debug!("Filtering for these IDs (because of env filter): {:?}", jids); sel.filter(schema::jobs::dsl::id.eq_any(jids)) } else { sel -- cgit v1.2.3 From fa5aecbc7df99982336916172bebd93cda294de6 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 9 Apr 2021 11:48:20 +0200 Subject: Add --limit option to "db jobs" Signed-off-by: Matthias Beyer --- src/cli.rs | 10 ++++++++++ src/commands/db.rs | 12 +++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/cli.rs b/src/cli.rs index 1473164..f2f5bf0 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -212,6 +212,16 @@ pub fn cli<'a>() -> App<'a> { .value_name("KV") .about("Filter for this \"key=value\" environment variable") ) + + .arg(Arg::new("limit") + .required(false) + .multiple(false) + .long("limit") + .short('L') + .takes_value(true) + .value_name("LIMIT") + .about("Only list newest jobs instead of all") + ) ) .subcommand(App::new("job") diff --git a/src/commands/db.rs b/src/commands/db.rs index 7dbf08a..ba66c14 100644 --- a/src/commands/db.rs +++ b/src/commands/db.rs @@ -11,6 +11,7 @@ use std::io::Write; use std::path::PathBuf; use std::process::Command; +use std::str::FromStr; use anyhow::Context; use anyhow::Error; @@ -342,8 +343,17 @@ fn jobs(conn_cfg: DbConnectionConfig, matches: &ArgMatches) -> Result<()> { sel }; - let data = sel.load::<(models::Job, models::Submit, models::Endpoint, models::Package)>(&conn)? + let sel = if let Some(limit) = matches.value_of("limit").map(i64::from_str).transpose()? { + sel.limit(limit) + } else { + sel + }; + + let data = sel + .order_by(schema::jobs::id.desc()) // required for the --limit implementation + .load::<(models::Job, models::Submit, models::Endpoint, models::Package)>(&conn)? .into_iter() + .rev() // required for the --limit implementation .map(|(job, submit, ep, package)| { let success = crate::log::ParsedLog::build_from(&job.log_text)? .is_successfull() -- cgit v1.2.3