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

support embedded schema for cassandra and sql update-schema #5195

Merged
merged 23 commits into from
Dec 5, 2023

Conversation

carlydf
Copy link
Contributor

@carlydf carlydf commented Dec 4, 2023

What changed?

This change adds a flag to temporal-sql-tool and temporal-cassandra-tool's update-schema and setup-schema to specify an embedded schema instead of a path to the schema directory or schema file.

NAME:
   temporal-cassandra-tool update-schema - update cassandra schema to a specific version

USAGE:
   temporal-cassandra-tool update-schema [command options] [arguments...]

OPTIONS:
   --version value, -v value      target version for the schema update, defaults to latest
   --schema-dir value, -d value   path to directory containing versioned schema
   --schema-name value, -s value  name of embedded versioned schema, one of: [cassandra/temporal cassandra/visibility]
NAME:
   temporal-sql-tool update-schema - update sql schema to a specific version

USAGE:
   temporal-sql-tool update-schema [command options] [arguments...]

OPTIONS:
   --version value, -v value      target version for the schema update, defaults to latest
   --schema-dir value, -d value   path to directory containing versioned schema
   --schema-name value, -s value  name of embedded versioned schema, one of: [mysql/v57/temporal mysql/v57/visibility mysql/v8/temporal mysql/v8/visibility postgresql/v96/temporal postgresql/v96/visibility postgresql/v12/temporal postgresql/v12/visibility]

Why?

We want to embed the db schemas in the temporal-sql-tool and temporal-cassandra-tool with go:embed so that customers can simply specify which schema they want using a flag instead of having to copy the schema files to wherever they're running the db tool and passing the path to the schema.

#2059

How did you test it?

I wrote a new unit test for reading the embedded file system, and the efs==nil codepath of the changed functions are all tested by the existing unit tests. I also updated the unit test for config validation to reflect the new changes.

I tested this locally with a locally running Postgres. Since the more complex changes are in updatetask.go, which is shared by the sql and the cassandra tool, I think there is no need to also test with a real cassandra db. I also built the cassandra command line tool and confirmed that the flags behaved as expected.

Potential risks

Schema updates could fail. But the changes are pretty contained to the new -s flag which isn't used anywhere in production yet, so the risk is very low. The code for the -d flag remained basically the same.

Is hotfix candidate?

No

@carlydf carlydf requested a review from a team as a code owner December 4, 2023 19:20
@CLAassistant
Copy link

CLAassistant commented Dec 4, 2023

CLA assistant check
All committers have signed the CLA.

@tdeebswihart
Copy link
Contributor

Do the setup-schema commands need to be updated as well? Seems like they should be

@carlydf
Copy link
Contributor Author

carlydf commented Dec 4, 2023

Do the setup-schema commands need to be updated as well? Seems like they should be

thanks for catching! i missed this cuz the schema path is not required for that command, and everywhere we use it we always first run setup-schema -v 0.0 followed by update-schema, but you're totally right, i will go add :)

schema/embed.go Outdated Show resolved Hide resolved
schema/embed.go Outdated Show resolved Hide resolved
schema/embed.go Outdated Show resolved Hide resolved
tools/common/schema/setuptask.go Outdated Show resolved Hide resolved
tools/common/schema/setuptask.go Outdated Show resolved Hide resolved
tools/common/schema/updatetask.go Outdated Show resolved Hide resolved
tools/common/schema/updatetask.go Outdated Show resolved Hide resolved
tools/common/schema/updatetask.go Outdated Show resolved Hide resolved
tools/common/schema/updatetask.go Outdated Show resolved Hide resolved
tools/common/schema/handler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tdeebswihart tdeebswihart left a comment

Choose a reason for hiding this comment

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

Thanks for working with me on some of this cleanup even though it wasn't from your work in the first place. The tool will be in a much better state after this!

tools/common/schema/handler.go Outdated Show resolved Hide resolved
tools/common/schema/handler.go Outdated Show resolved Hide resolved
@carlydf carlydf merged commit 46e9039 into main Dec 5, 2023
10 checks passed
@carlydf carlydf deleted the cdf/oss-435-embed-schema branch December 5, 2023 20:22
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