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

don't require resolveip if it won't be used #3251

Merged
merged 1 commit into from
May 29, 2024

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented May 12, 2024

  • The Jira issue number for this PR is: MDEV-______

Description

This avoids checking for resolveip if it won't be used by using the same conditional used before deciding to execute resolveip. I ran into this while reducing mariadb's installation footprint.

Release Notes

How can this PR be tested?

Install without resolveip?

Basing the PR against the correct MariaDB version

Eh I'm not really sure which one this falls under.

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@robinnewhouse
Copy link
Contributor

Hi @mscdex, thanks for the contribution!

  • Could you also add an MTR test showing the behaviour with different options (--force, etc.)?
  • I'm also curious if this results in measurable improvements in installation performance. Do you have any results showing total installation time?

@mscdex
Copy link
Contributor Author

mscdex commented May 14, 2024

  • Could you also add an MTR test showing the behaviour with different options (--force, etc.)?

I'm not sure exactly what you're proposing here. Can you explain a bit more?

  • I'm also curious if this results in measurable improvements in installation performance. Do you have any results showing total installation time?

I do not. I was more concerned about just being able to successfully initialize mariadb for the first time. I don't utilize hostnames for users, so either way it's a non-issue for me.

@robinnewhouse
Copy link
Contributor

I guess the question is, what problem does this solve? when you say "reducing mariadb's installation footprint", how does that manifest?

I suggest an MTR test as it's a useful way to show the behaviour your change fixes. I see there is one for testing the mysql_install_db script on windows, so that might provide a starting point. https://github.com/MariaDB/server/blob/11.5/mysql-test/main/mysql_install_db_win.test

@mscdex
Copy link
Contributor Author

mscdex commented May 15, 2024

I guess the question is, what problem does this solve?

I'm having to patch the various CMakeFiles in order to remove stuff that I don't need/want in order to reduce the overall footprint of MariaDB since there are no flags to toggle all of the things I'm removing. resolveip ends up (indirectly) being one of those casualties as a result.

Either way, IMO it makes sense to not check that something exists if it won't be used (when any of those special conditions are met).

I suggest an MTR test as it's a useful way to show the behaviour your change fixes.

The problem is this would require deleting/moving resolveip and ensuring mysql_install_db doesn't fail. After looking through the documentation and some of the existing tests it's not immediately clear how that would be accomplished. Not only that, I wasn't even able to find a definitive list of variables (especially for all relevant directories so I would know how to properly delete/move resolveip, as that would depend on installation prefixes and such) exposed to all tests. On top of that, I've never written a line of Perl in my life.

scripts/mysql_install_db.sh Outdated Show resolved Hide resolved
Copy link
Member

@vuvova vuvova left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@vuvova
Copy link
Member

vuvova commented May 25, 2024

Are you ok with this being pushed into 11.5? Or do you need it in an earlier version? This is basically a bug fix, it could go into 10.5, if you'd rebase it onto 10.5 or if you'd let me rebase it.

@mscdex
Copy link
Contributor Author

mscdex commented May 25, 2024

Are you ok with this being pushed into 11.5? Or do you need it in an earlier version? This is basically a bug fix, it could go into 10.5, if you'd rebase it onto 10.5 or if you'd let me rebase it.

I'm fine with rebasing it on 10.11, which is the current LTS, however will it propagate forward (to the next LTS branch too)? Especially in this case since there is a merge conflict when rebasing on 10.11, so I'm not sure which side I should be pulling from.

@vuvova
Copy link
Member

vuvova commented May 25, 2024

Yes, it will propagate. If you'd like, I can take your commit and push it into 10.11

@CLAassistant
Copy link

CLAassistant commented May 25, 2024

CLA assistant check
All committers have signed the CLA.

@mscdex mscdex changed the base branch from 11.5 to 10.11 May 25, 2024 17:33
@mscdex
Copy link
Contributor Author

mscdex commented May 25, 2024

I've rebased against 10.11.

@vuvova vuvova merged commit a960e95 into MariaDB:10.11 May 29, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants