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

MDEV-35847 mariadb client --wait flag broken #3763

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from

Conversation

DaveGosselin-MariaDB
Copy link
Member

Wire up the --wait flag in the mariadb client.

@vuvova
Copy link
Member

vuvova commented Jan 14, 2025

kind of risky to enable it after it was disabled for 23 years. I'd rather keep the status quo and remove it completely

@vuvova
Copy link
Member

vuvova commented Jan 14, 2025

although it works in mariadb-admin

@DaveGosselin-MariaDB
Copy link
Member Author

I'm curious why the last committer intended to delete this option because the rest of the logic for it is in place. It would've been trivial to assume false everywhere that wait_flag is used and simplified the code accordingly.

@vuvova
Copy link
Member

vuvova commented Jan 15, 2025

I'm not sure the last committer has actually intended that. It's quite possibly could've been a mistake.

I don't have a strong opinion about it. It was a dead code for 23 years, so clearly nobody cares whether it works or not and it could be safely deleted. On the other hand, mariadb-admin has it (and it works there), so for consistency mariadb could have it too.

@DaveGosselin-MariaDB
Copy link
Member Author

I'm biased (since I wrote the patch 😇 ) but really it's up to you. I'm happy to finish removing the flag, but like you said it would be nice for feature symmetry between the two clients (and maybe someone is using it for the admin client).

Wire up the --wait flag in the mariadb client.
@vuvova
Copy link
Member

vuvova commented Jan 15, 2025

you wrote you need it for some tooling, so it's plausible that someone else might need it too. let's keep it

@DaveGosselin-MariaDB
Copy link
Member Author

OK 😄 once you approve the PR then I'll merge. The target branch is 10.5, let me know if you'd rather it go somewhere else instead (11.8?).

@@ -1732,7 +1732,7 @@ static struct my_option my_long_options[] =
GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0},
{"vertical", 'E', "Print the output of a query (rows) vertically.",
&vertical, &vertical, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
{"wait", 'w', "Wait and retry if connection is down.", 0, 0, 0, GET_NO_ARG,
{"wait", 'w', "Wait and retry if connection is down.", &wait_flag, 0, 0, GET_BOOL,
Copy link
Member

Choose a reason for hiding this comment

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

that's not how it used to work. see commit c9d0428 (wait time was configurable, retrying forever)

and it's not how -w works in mariadb-admin (wait time is hard-coded, 5 seconds, number of retries is configurable)

For consistency, I suppose, it should do what mariadb-admin does

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll make the changes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants