mysql: wire up the mysql datastore engine to the CLI #532

sbryant · 2022-04-14T16:49:55Z

This PR adds the mysql datastore options to the CLI.

add datastore-engine mysql
datastore-mysql-table-prefix support
spicedb migrate support

pkg/cmd/datastore/datastore.go

pkg/cmd/migrate.go

vroldanbet · 2022-04-14T17:02:17Z

pkg/cmd/datastore/datastore.go

+		mysql.MaxOpenConns(opts.MaxOpenConns),
+		mysql.RevisionFuzzingTimedelta(opts.RevisionQuantization),
+		mysql.TablePrefix(opts.TablePrefix),
+		mysql.EnablePrometheusStats(),


@sbryant I ended up enabling by default the prometheus stats, I couldn't figure out how to wire it up with EnableDatastoreMetrics, just like the PSQL driver recently did.

@ecordell since you introduced that internal flag, is that something that can be passed as an ENV variable via CLI? I couldn't figure out why it wasn't being parsed.

The way I read the cobrautil setup is that a flag still needs to be defined for the env var approach to work? https://github.com/jzelinskie/cobrautil/blob/main/cobrautil.go#L38

Correct - right now that option is not bound to a cli flag. If you bind EnableDatastoreMetrics to a flag in RegisterDatastoreFlags then you'll be able to set it via CLI or env vars

@ecordell how does that flag even work then? How are metrics ever turned on?

@authzed test ping

@sbryant It wasn't being used anywhere, it was a flag we could turn on during tests if we wanted to (via code)

@sbryant It wasn't being used anywhere, it was a flag we could turn on during tests if we wanted to (via code)

Ahh okay! Thanks for the explanation

@ecordell as it stands, it means that postgresql prometheus metrics are off by default, and there is no way to turn them on via the CLI? Is that intentional? They used to be on by default so that would be a regression for any customers running PSQL

Sounds like this may need to be addressed, but a followup should do

@vroldanbet You're right, they were on by default before. I've filed #549 to follow up and change the default

vroldanbet

LGTM, we can follow up on the prometheus flag. Deferring to @authzed/spicedb-maintainers

pkg/cmd/migrate.go

sbryant · 2022-04-20T17:07:54Z

@josephschorr i believe this is ready to go, is there anything else missing?

Co-authored-by: Alex Slepak <ams11@github.com> Co-authored-by: Christopher Kirkland <chriskirkland@github.com> Co-authored-by: Hannah Gould <hagould@github.com> Co-authored-by: John Reed <johnpreed@github.com> Co-authored-by: Víctor Roldán Betancort <vroldanbet@github.com>

mysql docs recommend using utf8mb3 since ut8 will eventually become an alias for utf8mb4 https://dev.mysql.com/doc/refman/8.0/en/charset-unicode-utf8.html

josephschorr

LGTM

sbryant · 2022-04-20T22:36:59Z

The failures appear to be unrelated to this PR. I can't seem to force an retry workflow either :(

josephschorr · 2022-04-21T01:55:52Z

I've rerun them

vroldanbet

found a bug, let's fix on a follow up!

vroldanbet · 2022-04-21T10:41:45Z

pkg/cmd/migrate.go

+		migrationDriver, err = mysqlmigrations.NewMySQLDriverFromDSN(dbURL, tablePrefix)
+		if err != nil {
+			log.Fatal().Err(err).Msg("unable to create migration driver")
+		}


@sbryant I believe there is a bug here: manager is not being set to the MySQL migration manager.

follow up in #550

sbryant requested a review from a team as a code owner April 14, 2022 16:49

github-actions bot added the area/CLI Affects the command line label Apr 14, 2022

josephschorr self-requested a review April 14, 2022 16:55

josephschorr reviewed Apr 14, 2022

View reviewed changes

pkg/cmd/datastore/datastore.go Outdated Show resolved Hide resolved

pkg/cmd/migrate.go Outdated Show resolved Hide resolved

pkg/cmd/migrate.go Show resolved Hide resolved

vroldanbet reviewed Apr 14, 2022

View reviewed changes

vroldanbet previously approved these changes Apr 14, 2022

View reviewed changes

sbryant force-pushed the mysql-datastore-cli branch from 10172f4 to d0d1a4d Compare April 14, 2022 19:18

josephschorr dismissed vroldanbet’s stale review via d0d1a4d April 14, 2022 19:32

josephschorr reviewed Apr 15, 2022

View reviewed changes

pkg/cmd/migrate.go Outdated Show resolved Hide resolved

sbryant force-pushed the mysql-datastore-cli branch from 74e9e3f to 2ada77a Compare April 18, 2022 17:56

github-actions bot added the area/datastore Affects the storage system label Apr 20, 2022

sbryant force-pushed the mysql-datastore-cli branch from 7ee66cf to 85eb5f4 Compare April 20, 2022 17:05

sbryant and others added 6 commits April 20, 2022 17:48

advertise spanner support

711fed2

reoder mysql datastore option

a101d6c

regenerate datastore options

8839406

Update head flag to include spanner

c0048d5

utf8 is an alias to utf8mb3, which is deprecated

26f7e29

mysql docs recommend using utf8mb3 since ut8 will eventually become an alias for utf8mb4 https://dev.mysql.com/doc/refman/8.0/en/charset-unicode-utf8.html

josephschorr force-pushed the mysql-datastore-cli branch from 85eb5f4 to 26f7e29 Compare April 20, 2022 21:48

josephschorr approved these changes Apr 20, 2022

View reviewed changes

josephschorr enabled auto-merge April 20, 2022 21:54

josephschorr merged commit fe5fd1a into authzed:main Apr 21, 2022

github-actions bot locked and limited conversation to collaborators Apr 21, 2022

vroldanbet deleted the mysql-datastore-cli branch April 21, 2022 07:25

vroldanbet reviewed Apr 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql: wire up the mysql datastore engine to the CLI #532

mysql: wire up the mysql datastore engine to the CLI #532

sbryant commented Apr 14, 2022

vroldanbet Apr 14, 2022

sbryant Apr 14, 2022 •

edited

ecordell Apr 14, 2022

sbryant Apr 14, 2022

vroldanbet Apr 14, 2022

ecordell Apr 14, 2022

sbryant Apr 14, 2022

vroldanbet Apr 14, 2022

josephschorr Apr 20, 2022

ecordell Apr 20, 2022 •

edited

vroldanbet left a comment

sbryant commented Apr 20, 2022

josephschorr left a comment

sbryant commented Apr 20, 2022

josephschorr commented Apr 21, 2022

vroldanbet left a comment

vroldanbet Apr 21, 2022

vroldanbet Apr 21, 2022

mysql: wire up the mysql datastore engine to the CLI #532

mysql: wire up the mysql datastore engine to the CLI #532

Conversation

sbryant commented Apr 14, 2022

Choose a reason for hiding this comment

sbryant Apr 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell Apr 20, 2022 • edited

Choose a reason for hiding this comment

vroldanbet left a comment

Choose a reason for hiding this comment

sbryant commented Apr 20, 2022

josephschorr left a comment

Choose a reason for hiding this comment

sbryant commented Apr 20, 2022

josephschorr commented Apr 21, 2022

vroldanbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbryant Apr 14, 2022 •

edited

ecordell Apr 20, 2022 •

edited