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

Consolidate fetching of MySQL server info #1229

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

timvaillancourt
Copy link
Collaborator

@timvaillancourt timvaillancourt commented Dec 12, 2022

Description

This PR consolidates the many queries and functions that fetch MySQL server runtime configuration into a single struct and function: mysql.ServerInfo and mysql.GetServerInfo(...) (*ServerInfo, error) respectively. This reduces the server-info-related MySQL queries from 6 -> 2

Now server information for both the applier and inspector are stored in the migration context as single structs

Notes:

  • To handle cases where @@global.port and @@global.extra_port are NULL a gosql.NullInt64 was used when cleaning up base.ValidateConnection(...)
    • This required Port from mysql.InstanceKey to become int64 as there is no gosql.NullInt
  • Removed duplicate base.ValidateConnection(...) on the applier's this.singletonDB database handle as it is the same host as the already-validated this.db handle
    • This resolves these confusing/duplicate log lines:
      2022-12-10 04:20:29 INFO applier connection validated on primary:3306
      2022-12-10 04:20:29 INFO applier connection validated on primary:3306
  • The Applier initiated on %+v, version %+v log-lines now include the @@global.version_comment (in brackets) for more info
  • Unit test added to base.ValidateConnection(...) now that it takes in serverInfo *mysql.ServerInfo vs a harder-to-test db *sql.DB
  • Moved migrationContext.RequiresBinlogFormatChange(...) to inspector because it's never called outside of inspector
    • Added unit test
  • Remove unused .validateAndReadTimeZone() func from applier

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

@ramshad-sha ramshad-sha mentioned this pull request Mar 25, 2024
Closed
@meiji163
Copy link
Contributor

HI @timvaillancourt 👋 do you still want to add this one to 1.1.7 or should we push it out? I'm planning to cut a release this week or next

@meiji163 meiji163 modified the milestones: v1.1.7, v1.1.8 Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants