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.

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

fix: inherit configuration in run_parallel #215

Merged
merged 4 commits into from
Jun 20, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ jobs:
uses: actions-rs/cargo@v1
with:
command: install
args: cargo-semver-checks --version ^0.27 --locked
args: cargo-semver-checks --version ^0.32 --locked
- name: Check semver
run: |
cargo semver-checks check-release -p sqllogictest
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

## [0.20.5] - 2024-06-20

* fix(runner): when running in parallel, the runner will correctly inherit configuration like `sort_mode` and `labels` from the main runner.
BugenZhao marked this conversation as resolved.
Show resolved Hide resolved

## [0.20.4] - 2024-06-06

* bump dependencies
Expand Down
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ resolver = "2"
members = ["sqllogictest", "sqllogictest-bin", "sqllogictest-engines", "tests"]

[workspace.package]
version = "0.20.4"
version = "0.20.5"
edition = "2021"
homepage = "https://github.com/risinglightdb/sqllogictest-rs"
keywords = ["sql", "database", "parser", "cli"]
Expand Down
21 changes: 18 additions & 3 deletions sqllogictest/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,8 @@ impl<D: AsyncDB, M: MakeConnection<Conn = D>> Runner<D, M> {
};

if is_background {
// Spawn a new process, but don't wait for stdout, otherwise it will block until the process exits.
// Spawn a new process, but don't wait for stdout, otherwise it will block until
// the process exits.
let error: Option<AnyError> = match cmd.spawn() {
Ok(_) => None,
Err(e) => Some(Arc::new(e)),
Expand Down Expand Up @@ -1090,6 +1091,9 @@ impl<D: AsyncDB, M: MakeConnection<Conn = D>> Runner<D, M> {

/// accept the tasks, spawn jobs task to run slt test. the tasks are (AsyncDB, slt filename)
/// pairs.
// TODO: This is not a good interface, as the `make_conn` passed to `new` is unused but we
// accept a new `conn_builder` here. May change `MakeConnection` to support specifying the
// database name in the future.
Comment on lines +1094 to +1096
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the comment here. Where's the make_conn passed to new?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, do you mean self.conn?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so it seems self.conn is used to CREATE DATABASE, but we need new conns for individual databases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea of conn_builder here has something common with make_conn passed to new, but they're not compatible and seem to be kind of redundant to me. For example,

  • The first arg is a host randomly picked from the hosts, which is strange. If the callers want load balancing between hosts, they are allowed to capture a slice of hosts and do random picking in the body of make_conn or conn_builder.

  • The procedures to establish a connection to the database in make_conn and conn_builder are likely to the same, except for targeting different databases. We should unify them by changing the definition of MakeConnection. Only using the one passed in new to create databases in parallel mode does not seem to be a good design.

pub async fn run_parallel_async<Fut>(
&mut self,
glob: &str,
Expand All @@ -1115,9 +1119,20 @@ impl<D: AsyncDB, M: MakeConnection<Conn = D>> Runner<D, M> {
.await
.expect("create db failed");
let target = hosts[idx % hosts.len()].clone();

let mut tester = Runner {
conn: Connections::new(move || {
conn_builder(target.clone(), db_name.clone()).map(Ok)
}),
validator: self.validator,
column_type_validator: self.column_type_validator,
substitution: self.substitution.clone(),
sort_mode: self.sort_mode,
hash_threshold: self.hash_threshold,
labels: self.labels.clone(),
};

tasks.push(async move {
let mut tester =
Runner::new(move || conn_builder(target.clone(), db_name.clone()).map(Ok));
let filename = file.to_string_lossy().to_string();
tester.run_file_async(filename).await
})
Expand Down
6 changes: 3 additions & 3 deletions sqllogictest/src/substitution.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
use std::sync::OnceLock;
use std::sync::{Arc, OnceLock};

use subst::Env;
use tempfile::{tempdir, TempDir};

/// Substitute environment variables and special variables like `__TEST_DIR__` in SQL.
#[derive(Default)]
#[derive(Default, Clone)]
pub(crate) struct Substitution {
/// The temporary directory for `__TEST_DIR__`.
/// Lazily initialized and cleaned up when dropped.
test_dir: OnceLock<TempDir>,
test_dir: Arc<OnceLock<TempDir>>,
}

impl<'a> subst::VariableMap<'a> for Substitution {
Expand Down
Loading