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

PT-2340: support MySQL 8.4 #871

Open
wants to merge 35 commits into
base: 3.x
Choose a base branch
from
Open

Conversation

svetasmirnova
Copy link
Collaborator

@svetasmirnova svetasmirnova commented Oct 4, 2024

This huge PR adds MySQL 8.4 support to the Percona Toolkit.

  • The contributed code is licensed under GPL v2.0
  • Contributor Licence Agreement (CLA) is signed
  • util/update-modules has been ran
  • Documentation updated
  • Test suite update

- Adjusted sandbox scripts, so they can start MySQL 8.4
- Added MySQL 8.4 configuration file
- Removed empty file sandbox/5.6
- Removed unused files sandbox/set-mysql, sandbox/slave_channels_t.sql, sandbox/jenkins-test
- Removed offensive terminology from the sandbox scripts wherever it is possible
- Fixed status check for 8.4 sandbox
- Removed option "reset" from the usage output, because option itself was removed by 92a3671
- Fixed sandbox/start-sandbox script, so it can start custom setups with 8.4
- Removed offensive terminology from library files and their tests
- Removed unused sandbox/prove2junit.pl
- Added option mysql_ssl to DSN and possibility to have DSN of multiple letters
- Removed all unneeded occuriences of the word "master" in lib
- Removed lib/Percona/Test.pm, lib/Safeguards.pm, t/lib/Safeguards.t, because they are not used anymore
- Removed word "slave" from lib
- Removed runtime.txt after discussion with Anastasia Alexandrova
- Added "use VersionParser" into tests in t/lib when needed
- Removed word master from tests for pt-archiver, pt-config-diff, pt-deadlock-logger, pt-duplicate-key-checker, pt-find, pt-fk-error-logger, pt-heartbeat, pt-index-usage, pt-ioprofile, pt-kill, pt-mysql-summary
- Removed word slave from tests for pt-archiver, pt-config-diff, pt-deadlock-logger, pt-duplicate-key-checker, pt-find, pt-fk-error-logger, pt-heartbeat, pt-index-usage, pt-ioprofile, pt-kill, pt-mysql-summary
- Updated modules for pt-archiver, pt-config-diff, pt-deadlock-logger, pt-duplicate-key-checker, pt-find, pt-fk-error-logger, pt-heartbeat, pt-index-usage, pt-ioprofile, pt-kill, pt-mysql-summary
- Changed mysql_ssl patch, so it is now short option s
- Added a check for existing zombies in t/pt-kill/execute_command.t
- Added bin/pt-galera-log-explainer to .gitignore
- Updated modules and tests for pt-online-schema-change
- Removed typo from lib/MasterSlave.pm
- Updated modules and tests for pt-query-digest, pt-show-grants, pt-slave-delay, pt-slave-find, pt-slave-restart, pt-stalk, pt-summary
- Updated modules and tests for pt-query-digest, pt-show-grants, pt-slave-delay, pt-slave-find, pt-slave-restart, pt-stalk, pt-summary, pt-table-checksum
- Updated modules and tests for pt-table-sync, pt-table-usage, pt-upgrade, pt-variable-advisor
- Fixed staff I broke for 8.0
- Fixed 5.7 tests
- Added deprecation warning to pt-slave-delay
- Re-enabled tests for pt-slave-delay to check the warning
- Disabled pt-slave-delay for MySQL 8.1+
- Created pt-replica-restart. pt-slave-restart kept as symbolic link
- Changed write-user-docs script, so it creates special documentation pages for symbolic links.
- Fixed comparison operators for VersionCheck calls
- Created pt-replica-find. pt-slave-find kept as symbolic link
- Updated documentation
- Fixed typos
- Added tests for MyRocks and TokuDB clustered indexes into t/pt-duplicate-key-checker/clustered_keys.t
- Updated modules
- Updated tests for pt-variable-advisor and lib/VariableAdvisorRules
- Moved data collection for THP from lib/bash/report_system_info.sh to lib/bash/collect_system_info.sh, so pt-summary reports THP status on the machine where samples were collected, not on the machine where an engineer examines samples.
- Adjusted test results, broken due to mysql.component table in PS
- Tests for the minimal SSL support
- Updated util/update-modules, so they don't skip tools with modules, not defined in lib
- Added tests for legacy style heartbeat and checksum tables
- Added test for replica lag check for pt-archiver
- Re-added deprecated slave- options
- Added tests for deprecation warnings and for legacy options for pt-archiver
- Removed practically not supported options --replica-user and --replica-password from pt-archiver, pt-kill, pt-query-digest
- Added legacy source/replica options (master/slave) variants and tests for pt-heartbeat, pt-online-schema-change, pt-replica-find, pt-replica-restart, pt-table-checksum, pt-table-sync
- Updated modules after lib/MasterSlave.pm change
- Adjusted t/pt-deadlock-logger/check_schema_exists.t so it makes sense
- Adjusted pt-variable-advisor, so it reflects current defaults
bin/pt-archiver Outdated
@@ -7179,14 +7292,14 @@ sub main {
my $lag_dbh = $lag_server->{'dbh'};
my $id = $lag_server->{'id'};
if ( $lag_dbh ) {
my $lag = $ms->get_slave_lag($lag_dbh);
my $lag = $ms->get_replica_lag($lag_dbh);
while ( !defined $lag || $lag > $o->get('max-lag') ) {
PTDEBUG && _d("Sleeping: slave lag for server '$id' is", $lag);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since pt-heartbeat had his schema changed to reflect the source/replica change, does this one should be changed for coherence ?
Or, is it left as is so to keep the current output ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi! I don't understand your comment. Modified code uses get_replica_lag. What needs to be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is for _d("Sleeping: slave lag for server '$id' is", $lag);, on row 7297 and 7299 (on new branch).
7297 is with PTDEBUG, so really not a problem, but the other one is a log to users
"replica lag" would make it more coherent, but that's only a nitpick

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, please review again

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm for the bin, the tests will need some fixing as well

$ grep -rn 'Sleeping:'
t/pt-archiver/replica_lag.t:73:   qr/Sleeping: slave lag for server/,
t/pt-archiver/replica_lag.t:101:   qr/Sleeping: slave lag for server/,
t/pt-archiver/replica_lag.t:129:   qr/Sleeping: slave lag for server/,
t/pt-archiver/replica_lag.t:156:   qr/Sleeping: slave lag for server/,
t/pt-archiver/replica_lag.t:184:   qr/Sleeping: slave lag for server/,
t/pt-archiver/replica_lag.t:212:   qr/Sleeping: slave lag for server/,
t/pt-archiver/replica_lag.t:240:   qr/Sleeping: slave lag for server/,
bin/pt-archiver:7297:                  PTDEBUG && _d("Sleeping: replica lag for server '$id' is", $lag);
bin/pt-archiver:7299:                     _d("Sleeping: replica lag for server '$id' is", $lag);

- s/slave lag/replica lag/ in reports
@@ -75,7 +75,7 @@ use constant {
COM_BINLOG_DUMP => '12',
COM_TABLE_DUMP => '13',
COM_CONNECT_OUT => '14',
COM_REGISTER_SLAVE => '15',
COM_REGISTER_REPLICA => '15',
Copy link
Contributor

Choose a reason for hiding this comment

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

The packet exists on 8.0 and 8.4, but not 5.7
I don't find any code usage of it in any lib/bin though

our $source_reset = 'binary logs and gtids';
our $source_change = 'replication source';
our $replica_name = 'replica';
if ( $sandbox_version lt '8.1' || ( $ENV{FORK} || "" eq 'mariadb' ) ) {
Copy link
Contributor

@ylacancellera ylacancellera Oct 10, 2024

Choose a reason for hiding this comment

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

works in an unintended way (PT-2390). I listed this code path in PT-2390 for review. I'm still commenting since it's a different condition
10.x will return true on $sandbox_version lt '8.1', so the second part on the fork is not used.

$ echo $sandbox_version $FORK
10.4 mysql
$ perl -e "if ( $sandbox_version lt '8.1' || ( $ENV{FORK} || '' eq 'mariadb' ) ) { print('works') }"
works

As long as mysql 10 does not exist it still does what we want

# Check at the end of this package for the call to main() which actually runs
# the program.
# ###########################################################################
package pt_slave_find;
Copy link
Contributor

@ylacancellera ylacancellera Oct 11, 2024

Choose a reason for hiding this comment

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

can this impact maintenance ? The names are mismatched between the toolname and the package name
If it's safe, good to me. I don't find any direct usage

# +- 1
# +- 2
# +- 1
sub print_slaves {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since many internal variables were renamed in lib, I'm commenting to be thorough
This tool package did not receive the s/slave/replica
However I don't see any misusage

Unless this was a goal to remove all of them in this PR specifically, I'm ok to keep it and maybe flag it for later

}
push @lines, [
'Replication',
($ss ? "Is a slave, " : "Is not a slave, ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Unaligned with rest, I can flag it for later if needed

Version 5.1.34-log
Server ID 12346
Uptime 04:54 (started 2010-06-17T11:21:24)
Replication Is a slave, has 1 slaves connected
Copy link
Contributor

Choose a reason for hiding this comment

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

Unaligned with the rest, but coherent with the current states of things
Again, I can flag it for later, or skip

Copy link
Contributor

@ylacancellera ylacancellera left a comment

Choose a reason for hiding this comment

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

I have commented what I found already, it's only nitpicks on some missed "slave" wordings. (except the version comparisons, but PT-2390 have been created so I exclude it).

Looks good to me

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.

3 participants