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.

Sign up for GitHub

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

Jump to bottom

[Advanced Visibility with SQL] Adding MySQL 8 schema and interface #3552

Merged
merged 3 commits into from
Jan 31, 2023
Merged

[Advanced Visibility with SQL] Adding MySQL 8 schema and interface #3552

merged 3 commits into from
Jan 31, 2023

Conversation

rodrigozhou
Copy link
Contributor

@rodrigozhou rodrigozhou commented Nov 2, 2022 •
edited
Loading

What changed?
Added MySQL 8 schemas:

  • For main Temporal storage, it's just a copy from MySQL v57 schema
  • For visibility:
    • v1.0, v1.1: It's a copy from v5.7 (to support migration).
    • v1.2: Changes to support advanced visibility:
      • Added search attributes JSON map.
      • Added generated columns to extract the search attributes fields from JSON.
      • Deleted old indices and created new ones.

Created base MySQL 8 DB interface. It basically wraps the interface for v57, only overwriting the methods to check the right version.

Added config to start Temporal with MySQL 8 and added respective targets to set it up.

Why?
Advanced visibility with SQL databases.

How did you test it?
Started temporal using MySQL 8, and copied existing unit tests for MySQL to use the new interface.

Potential risks
No.

Is hotfix candidate?
No.

@rodrigozhou rodrigozhou requested a review from a team as a code owner November 2, 2022 22:17
@rodrigozhou rodrigozhou force-pushed the advanced-visibility-mysql-schema branch from cbd5bbc to d2aace2 Compare November 2, 2022 22:52
@rodrigozhou rodrigozhou requested a review from alexshtin November 3, 2022 06:23
@alexshtin alexshtin removed their request for review November 16, 2022 06:13
@rodrigozhou rodrigozhou force-pushed the advanced-visibility-mysql-schema branch 2 times, most recently from 1ecb3e9 to 308dd21 Compare January 20, 2023 02:15
@rodrigozhou rodrigozhou changed the title [Advanced Visibility with MySQL] Adding schema and DB interface [Advanced Visibility with SQL] Adding MySQL 8 schema and interface Jan 20, 2023
@rodrigozhou rodrigozhou requested a review from alexshtin January 20, 2023 02:34
@rodrigozhou rodrigozhou force-pushed the advanced-visibility-mysql-schema branch from 308dd21 to 622bc9e Compare January 23, 2023 19:13
MichaelSnowden
MichaelSnowden reviewed Jan 23, 2023
View reviewed changes
common/persistence/tests/mysql_test_util.go
@@ -56,8 +56,8 @@ const (

// TODO hard code this dir for now
// need to merge persistence test config / initialization in one place
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we're no longer testing v57?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In reality, we don't test against MySQL 5.7 anywhere. All docker container is using MySQL 8. What it means is that some tests won't run with the old visibility interface that supports MySQL v5.7.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we aren't testing MySQL 5.7 anywhere, but we provide a library for it, that's risky for our users. Fixing that is outside the scope of this PR, but do we have a follow-up task, like sunsetting 5.7 support altogether?

Copy link
Contributor Author

@rodrigozhou rodrigozhou Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For v1.21, we're removing support for MySQL 5.7.

MichaelSnowden
MichaelSnowden reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@MichaelSnowden MichaelSnowden left a comment

Choose a reason for hiding this comment

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

LGTM, just some questions

@rodrigozhou rodrigozhou force-pushed the advanced-visibility-mysql-schema branch 4 times, most recently from 5a9a71e to 3f7b1cb Compare January 28, 2023 08:45
@rodrigozhou rodrigozhou force-pushed the advanced-visibility-mysql-schema branch from 3f7b1cb to ccb725d Compare January 30, 2023 20:06
@rodrigozhou rodrigozhou merged commit fa429a5 into temporalio:master Jan 31, 2023
@rodrigozhou rodrigozhou deleted the advanced-visibility-mysql-schema branch January 31, 2023 02:02
@hiramhuang
Copy link

@rodrigozhou Is it possible to remove FOREIGN KEY from custom_search_attributes table in the future version? I'm asking this because I'm using Vitess version of MySQL which does not support foreign keys.

@rodrigozhou
Copy link
Contributor Author

@hiramhuang Sure, not a big deal. PR: #3937

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelSnowden MichaelSnowden MichaelSnowden approved these changes

@alexshtin alexshtin Awaiting requested review from alexshtin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rodrigozhou @hiramhuang @MichaelSnowden