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

Conversation

BugenZhao
Copy link
Collaborator

In Runner, we create a new runner for each parallelism without reusing self.conn, which is not good. What's worse is that we don't inherit other fields from self, resulting in add_label not taking effect when running in parallel.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao changed the title fix: reuse configuration in run_parallel fix: inherit configuration in run_parallel Jun 20, 2024
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

OK-ish as a quick fix

sqllogictest/src/connection.rs Outdated Show resolved Hide resolved
Comment on lines +1094 to +1096
// 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.
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.

CHANGELOG.md Show resolved Hide resolved
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao BugenZhao merged commit da5ef94 into main Jun 20, 2024
4 checks passed
@BugenZhao BugenZhao deleted the bz/parallel-runner-fields branch June 20, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants