feat(support-mysql-8): Added support for mysql version 8.0 and above #170

sairaj18 · 2024-12-26T06:29:24Z

The query cache variables are removed from MySQL 8.0 - https://dev.mysql.com/doc/refman/5.7/en/query-cache-status-and-maintenance.html - Due to this the following metrics will not be support for mysql version 8.0 and above

db.qCacheFreeBlocks
db.qCacheFreeMemoryBytes
db.qCacheHitRatio
db.qCacheHitsPerSecond
db.qCacheInserts
db.qCacheLowmemPrunesPerSecond
db.qCacheNotCachedPerSecond
db.qCacheQueriesInCachePerSecond
db.qCacheTotalBlocks
db.qCacheUtilization

From MySQL 8.0.23 the statement CHANGE MASTER TO is deprecated. The alias CHANGE REPLICATION SOURCE TO should be used instead. The parameters for the statement also have aliases that replace the term MASTER with the term SOURCE. For example, MASTER_HOST and MASTER_PORT can now be entered as SOURCE_HOST and SOURCE_PORT.
More Info - https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-23.html.

Code changes -

Updated the existing code to support MySQL versions b/w 5.6.0 - 8.0.0 and versions greater than 8.0.0
Updated unit test for the same
Added integration tests for mysql versions 8.0.40 and 9.1.0

Testing details here

rajrohanyadav

Thanks for taking care of this.

Left some comments. Some more points:

Also, a lot of new functions are added without unit tests. I think it makes sense to add tests for the ones that have logical statements.
Do you think it improves the code if we have an optional argument in config with db version? We won't have to compute version on each execution. If not specified, we anyways do it.
IMO, the additional dependency is not required. But we can discuss.
Can version throughout the code be replaced with dbVersion or something similar. It could be confused with integration version.

go.mod

Makefile

src/default_metrics.go

src/extended_metrics.go

src/default_metrics.go

src/extended_metrics.go

rajrohanyadav · 2024-12-30T07:26:09Z

src/extended_metrics.go

+}
+
+func threadCacheMissRate(metrics map[string]interface{}) (float64, bool) {
+	// TODO compute the value within the interval


I didn't understand the TODO statement here.

this is already existing comment that just moved to different file

We need to understand why it is there. If we think we don't need it, it can be removed.

rajrohanyadav · 2024-12-30T07:38:21Z

src/metrics-parse.go

+func extractSemver(version string) (string, error) {
+	reg := regexp.MustCompile(`^(?P<major>\d+)(?:\.(?P<minor>\d+))?(?:\.(?P<patch>\d+))?`)
+	matches := reg.FindStringSubmatch(version)
+
+	if len(matches) == 0 || matches[1] == "" {
+		return "", errSemanticVersionNotFound
+	}
+
+	major := matches[1]
+	minor := "0"
+	patch := "0"
+
+	if len(matches) > 2 && matches[2] != "" {
+		minor = matches[2]
+	}
+	if len(matches) > 3 && matches[3] != "" {
+		patch = matches[3]
+	}
+
+	return fmt.Sprintf("%s.%s.%s", major, minor, patch), nil
+}


I don't think we need this much complexity for getting the version. MySQL Doc specifies that the version might have a suffix but not a prefix. Therefore, you'll always get version before the first -.
Moreover, the documentation states: This function is unsafe for statement-based replication. A warning is logged if you use this function when [binlog_format](https://dev.mysql.com/doc/refman/8.4/en/replication-options-binary-log.html#sysvar_binlog_format) is set to STATEMENT. Though the binlog_format is deprecated, do you think it might cause any issues for older customer environments?

will discuss this with kai

I ran integration tests in github actions for version 5.7.35 with binlog_format=STATEMENT and it passed. I didn’t make any change to handle if we are not able to parse DB version output. So, We are getting the version as excepted without any warning with was mentioned in the Mysql doc.

I think we should just note this in the documentation. Other than that it seems fine.

src/slave_metrics.go

sairaj18 · 2024-12-31T11:18:08Z

Thanks for taking care of this.

Left some comments. Some more points:

Also, a lot of new functions are added without unit tests. I think it makes sense to add tests for the ones that have logical statements.

Do you think it improves the code if we have an optional argument in config with db version? We won't have to compute version on each execution. If not specified, we anyways do it.

IMO, the additional dependency is not required. But we can discuss.

Can version throughout the code be replaced with dbVersion or something similar. It could be confused with integration version.

Added unit tests for the new functions that have been introduced.
We can check the optional argument configuration with kai
removed the additional dependency as we discussed
replaced version with dbVersion in most of the code wherever needed

rajrohanyadav

LGTM

Just one point as mentioned in my last comment. It also provides a way to not break the integration if we are unable to get version.

Do you think it improves the code if we have an optional argument in config with db version? We won't have to compute version on each execution. If not specified, we anyways do it.

… the query output

sairaj18 marked this pull request as ready for review December 26, 2024 10:11

sairaj18 requested a review from a team as a code owner December 26, 2024 10:11

sairaj18 force-pushed the NR-133316-add-support-for-mysql-version-8-and-above branch from 25a2167 to 6d1792b Compare December 26, 2024 14:52

added support for mysql versions 8.0 and above

62e1c0a

sairaj18 force-pushed the NR-133316-add-support-for-mysql-version-8-and-above branch from 6d1792b to 62e1c0a Compare December 26, 2024 15:09

rajrohanyadav requested changes Dec 30, 2024

View reviewed changes

sairaj18 force-pushed the NR-133316-add-support-for-mysql-version-8-and-above branch from 1dc1da9 to 9a5c77a Compare December 31, 2024 03:47

resolved comments

fe29f92

sairaj18 force-pushed the NR-133316-add-support-for-mysql-version-8-and-above branch from 9a5c77a to fe29f92 Compare December 31, 2024 03:50

sairaj18 requested a review from rajrohanyadav December 31, 2024 11:29

sairaj18 force-pushed the NR-133316-add-support-for-mysql-version-8-and-above branch from 3754b63 to fe29f92 Compare December 31, 2024 11:57

ran to update the go.sum

5f06db8

sairaj18 force-pushed the NR-133316-add-support-for-mysql-version-8-and-above branch from fdeaa60 to 5f06db8 Compare January 2, 2025 12:10

rajrohanyadav previously approved these changes Jan 2, 2025

View reviewed changes

Use dbversion as 5.7.0 when failing to retrive using query or parsing…

4c7c043

… the query output

sairaj18 dismissed rajrohanyadav’s stale review via 4c7c043 January 3, 2025 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(support-mysql-8): Added support for mysql version 8.0 and above #170

feat(support-mysql-8): Added support for mysql version 8.0 and above #170

sairaj18 commented Dec 26, 2024 •

edited

Loading

rajrohanyadav left a comment

rajrohanyadav Dec 30, 2024

sairaj18 Dec 30, 2024

rajrohanyadav Jan 2, 2025

rajrohanyadav Dec 30, 2024

sairaj18 Dec 31, 2024

sairaj18 Dec 31, 2024

rajrohanyadav Jan 2, 2025

sairaj18 commented Dec 31, 2024 •

edited

Loading

rajrohanyadav left a comment

feat(support-mysql-8): Added support for mysql version 8.0 and above #170

Are you sure you want to change the base?

feat(support-mysql-8): Added support for mysql version 8.0 and above #170

Conversation

sairaj18 commented Dec 26, 2024 • edited Loading

rajrohanyadav left a comment

Choose a reason for hiding this comment

rajrohanyadav Dec 30, 2024

Choose a reason for hiding this comment

sairaj18 Dec 30, 2024

Choose a reason for hiding this comment

rajrohanyadav Jan 2, 2025

Choose a reason for hiding this comment

rajrohanyadav Dec 30, 2024

Choose a reason for hiding this comment

sairaj18 Dec 31, 2024

Choose a reason for hiding this comment

sairaj18 Dec 31, 2024

Choose a reason for hiding this comment

rajrohanyadav Jan 2, 2025

Choose a reason for hiding this comment

sairaj18 commented Dec 31, 2024 • edited Loading

rajrohanyadav left a comment

Choose a reason for hiding this comment

sairaj18 commented Dec 26, 2024 •

edited

Loading

sairaj18 commented Dec 31, 2024 •

edited

Loading