-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support for mariadb server config parameter #3763
base: master
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe changes in this pull request update the MariaDB add-on to version 2.7.2, introducing a new optional configuration parameter, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
mariadb/config.yaml (1)
26-27
: Approve the addition of theinnodb_buffer_pool_size
parameter.The addition of the
innodb_buffer_pool_size
parameter aligns with the PR objective and will allow users to optimize the performance of their MariaDB database by adjusting the buffer pool size. The default value of128M
is a reasonable starting point, but users with larger databases may need to adjust this value based on their specific requirements.Consider adding guidance in the documentation to help users determine the appropriate value for their use case. Factors to consider include:
- The size of the database
- The available memory on the system
- The expected workload and query patterns
Additionally, ensure that the documentation clearly explains the impact of this parameter on performance and provides instructions for monitoring and adjusting the value as needed.
Also applies to: 43-44
mariadb/DOCS.md (1)
79-79
: Explanation looks good! Consider a minor rephrasing.The explanation provides clear context and rationale for introducing the new
parameters.innodb_buffer_pool_size
parameter. It helpfully mentions the RAM availability caveat and includes a reference link to the related migration issues.Consider rephrasing the first sentence slightly to improve clarity:
-During Home Assistant schema updates some people experienced [errors][migration-issues] on large databases. +Some users have experienced [errors][migration-issues] during Home Assistant schema updates on large databases.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- mariadb/CHANGELOG.md (1 hunks)
- mariadb/DOCS.md (2 hunks)
- mariadb/config.yaml (3 hunks)
- mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/run (1 hunks)
Additional context used
Path-based instructions (3)
mariadb/config.yaml (6)
Pattern
*/**(html|markdown|md)
: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md)
: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md)
: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md)
: - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md)
: do not comment on HTML used for icons
Pattern
*/**(html|markdown|md)
: Avoid flagging inline HTML for embedding videos in future reviews for this repository.mariadb/CHANGELOG.md (6)
Pattern
*/**(html|markdown|md)
: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md)
: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md)
: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md)
: - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md)
: do not comment on HTML used for icons
Pattern
*/**(html|markdown|md)
: Avoid flagging inline HTML for embedding videos in future reviews for this repository.mariadb/DOCS.md (6)
Pattern
*/**(html|markdown|md)
: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md)
: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md)
: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md)
: - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md)
: do not comment on HTML used for icons
Pattern
*/**(html|markdown|md)
: Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Markdownlint
mariadb/CHANGELOG.md
5-5: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (6)
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/run (4)
7-8
: LGTM!Initializing an empty string to hold the concatenated parameters is a good approach. The variable name
param_string
clearly conveys its purpose.
10-15
: LGTM!The code segment correctly fetches the parameters using
bashio::config
and processes them with awhile
loop. The format--parameter
is a common convention for command line parameters.
17-17
: LGTM!Removing the trailing whitespace from
param_string
usingsed
is a good practice to ensure clean and consistent strings.
20-22
: LGTM!The changes to the logging statement and the execution command for
mysqld
are beneficial:
- Including the command line parameters in the logging statement enhances visibility into the startup process.
- Modifying the execution command to include
param_string
allows for greater flexibility in the service's configuration at runtime.mariadb/config.yaml (1)
2-2
: LGTM!The version update to 2.7.2 aligns with the PR objective and is expected to be backward compatible.
mariadb/DOCS.md (1)
77-78
: LGTM!The new optional configuration parameter
parameters.innodb_buffer_pool_size
is introduced correctly without breaking existing configurations. The parameter name is clear and follows the existing naming convention.
@coderabbitai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @agners
I saw that you were the last committer in the MariaDB addon, I'm not sure if anything else is to be done with this pull request for acceptance? The recommended edits by CodeRabbit were pushed, but seems there is no update to the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I see you came up with a somewhat generic approach, which is kinda interesting.
Does this work really? I wonder since the options according to mysqld --help --verbose
uses dashes and a equal sign (--innodb-buffer-pool-size=#
).
@@ -4,7 +4,19 @@ | |||
# Start MariaDB service | |||
# ============================================================================== | |||
|
|||
# Initialize an empty string to hold the concatenated parameters | |||
param_string="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be an array? I wonder if your approach of simply concatenating parameters would work with an argument which contains spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not have any more parameters in mind that should be adjustable as this was seemingly the one that caused the migration issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion to the required format is done in line 15
It worked in dev container:
|
Fixes #3754
Summary by CodeRabbit
New Features
parameters.innodb_buffer_pool_size
for improved server control.Documentation
innodb_buffer_pool_size
option and its implications for Home Assistant schema updates.Configuration Changes
config.yaml
to include the new parameterinnodb_buffer_pool_size
with a default value of128M
.