MDEV-38202: make init_rpl_role visible at runtime#4904
MDEV-38202: make init_rpl_role visible at runtime#4904Mahmoud-kh1 wants to merge 1 commit intoMariaDB:mainfrom
Conversation
f6f6a30 to
029860e
Compare
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please, in addition to the notes below:
- add a commit message to your commit explaining what it is that you do
- update the Jira with a complete description of what the new variable is (data type, validity period, purpose), when and how it can be set, etc. : helps documenting it.
- Make sure that there are no failing buildbot hosts.
|
|
||
| --echo # it should show in INFORMATION_SCHEMA.GLOBAL_VARIABLES | ||
| SELECT * FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES | ||
| WHERE VARIABLE_NAME = 'init_rpl_role'; No newline at end of file |
There was a problem hiding this comment.
Please always terminate the last line with a new line.
We also customarily add a --echo # End of MA.MI tests at the end: helps conflict resolution when merging up and down.
sql/sys_vars.cc
Outdated
| NOT_IN_BINLOG, ON_CHECK(check_init_string)); | ||
|
|
||
| #ifdef HAVE_REPLICATION | ||
| static Sys_var_enum Sys_init_rpl_role( |
There was a problem hiding this comment.
Feel free to leave that to the final reviewer. But: is there any specific reason this should be a system variable?
It's read only at runtime and not settable via the command line/config file at startup. So, in essence, you can only read it. So why not make this a status variable?
There was a problem hiding this comment.
I just make it system variable to support this syntax
SELECT @@global.init_rpl_role;
SELECT @@init_rpl_role;
There was a problem hiding this comment.
again, why? what's better about this syntax compared to SHOW GLOBAL STATUS LIKE 'init_rpl_role' ?
There was a problem hiding this comment.
I thought it will be just a nice bonus :) , I will turn it to status variable as you suggested.
There was a problem hiding this comment.
I stumbled upon the System & Status Variables Guide.
In general, I believe this should be a system variable, as it’s associated with a mode configuration rather than a status report.
In theory, it is possible to make --init-rpl-role mutable during runtime for recovering complex Semi-Sync networks/situations.
But in practice, --init-rpl-role doesn’t scale. (see also MDEV-34878)
There was a problem hiding this comment.
FWIW, I've requested that this is a status variable for the following reasons:
- I've assumed that a value will never be assigned directly to it.
- Status variables are more light-weight than system variables: less locking needed etc.
- I personally find a non-settable system variable to be an abomination and a gotcha: why would you put something non-settable in a settable variables namespace?
This said, it's your review and your codebase. So feel free to ignore the above or not.
sql/sys_vars.cc
Outdated
| "The replication role that the server was started with. " | ||
| "Possible values are MASTER and SLAVE", | ||
| READ_ONLY GLOBAL_VAR(init_rpl_role_val), NO_CMD_LINE, | ||
| rpl_role_type, DEFAULT(RPL_AUTH_MASTER)); |
There was a problem hiding this comment.
Do you really need a default here? You're going to always set it.
There was a problem hiding this comment.
I see rp_status has it as default value so I follow it
There was a problem hiding this comment.
What I implied is that you will never see this default value. So you might just as well put 0 here.
1979739 to
8795f3b
Compare
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for making it a status variable!
Some cleanups below. And then it's ready.
Problem: The --init-rpl-role option can be set in the .cnf file or command line to configure the server replication role at startup (MASTER or SLAVE), but there was no way to check this value at runtime. This made it hard to for investigation issues because we can't know which role the server was started with. Solution: I added a new read-only status variable init_rpl_role that captures the value of rpl_status right after startup and keeps it unchanged. ex usage: SHOW STATUS LIKE 'init_rpl_role'; SHOW GLOBAL STATUS LIKE 'init_rpl_role';
8795f3b to
648ee9c
Compare
sql/sys_vars.cc
Outdated
| NOT_IN_BINLOG, ON_CHECK(check_init_string)); | ||
|
|
||
| #ifdef HAVE_REPLICATION | ||
| static Sys_var_enum Sys_init_rpl_role( |
There was a problem hiding this comment.
I stumbled upon the System & Status Variables Guide.
In general, I believe this should be a system variable, as it’s associated with a mode configuration rather than a status report.
In theory, it is possible to make --init-rpl-role mutable during runtime for recovering complex Semi-Sync networks/situations.
But in practice, --init-rpl-role doesn’t scale. (see also MDEV-34878)
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Thank you for your work. Please stay tuned for the final review.
This PR adds a new feature to see the server’s initial replication role at runtime.
I added a new read-only status variable init_rpl_role that captures the value of rpl_status right after startup and keeps it unchanged.
Reason
Previously,
init_rpl_rolecould be set in the cnf file but was not visible in runtime which will be helpful for investigating issues as it will allow us to see with which value mariadb was started.Tests :
Added basic tests to verify that
init_rpl_roleis correctly set at startup and displayed properly.