Support Mysql #224

markphelps · 2020-02-07T19:27:23Z

MySQL is still a popular DB, so let's support it

derekperkins · 2020-05-18T16:43:53Z

This is a blocker in our evaluation of flipt

markphelps · 2020-05-21T13:31:35Z

@derekperkins I plan to try and tackle this over the long weekend coming up

sagikazarmark · 2020-06-03T08:58:16Z

Just an idea: maybe consider using an ORM for a DB storage implementation? There aren't many good ORMs for Go unfortunately, but this looks promising: https://github.com/facebookincubator/ent

Normally, I'm not a huge fan of ORMs because they hurt application architecture and gives less experienced developers the wrong ideas, but as an implementation of an interface they work great, because they allow you to work rapidly.

markphelps · 2020-06-04T16:03:37Z

@sagikazarmark thanks for the suggestion! i'll take a look for future use

The squirrel library im using to construct the queries does a pretty good job of abstracting the db specific stuff, so it's mainly just handling the different error codes that are thrown from the different drivers when there are constraint violations/etc.

I have a branch where im abstracting the sql code to be able to handle the specific errors more cleanly depending on the driver being used. I hope to have this finished this weekend.. just trying to find the time to work on it!

markphelps · 2020-06-20T23:30:29Z

@derekperkins @sagikazarmark I am extremely close to shipping this!.. However, I could actually use some help figuring out why a couple test cases are failing intermittently, but only when running with MySQL.

Would either of you (or anyone else) be able to help? My MySQL skills are a bit rusty.

You can see one of the failures in this Action run: https://github.com/markphelps/flipt/runs/791147606?check_suite_focus=true#step:6:632

And here's another time when running locally:

=== RUN   TestGetEvaluationDistributions
    TestGetEvaluationDistributions: evaluation_test.go:171: 
        	Error Trace:	evaluation_test.go:171
        	Error:      	Not equal: 
        	            	expected: "40c07e69-dff1-41df-84c8-3f701c965577"
        	            	actual  : "740b2abd-b474-451d-b3e5-70f953f38ae5"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-40c07e69-dff1-41df-84c8-3f701c965577
        	            	+740b2abd-b474-451d-b3e5-70f953f38ae5
        	Test:       	TestGetEvaluationDistributions
    TestGetEvaluationDistributions: evaluation_test.go:172: 
        	Error Trace:	evaluation_test.go:172
        	Error:      	Not equal: 
        	            	expected: "foo"
        	            	actual  : "bar"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-foo
        	            	+bar
        	Test:       	TestGetEvaluationDistributions
    TestGetEvaluationDistributions: evaluation_test.go:177: 
        	Error Trace:	evaluation_test.go:177
        	Error:      	Not equal: 
        	            	expected: "740b2abd-b474-451d-b3e5-70f953f38ae5"
        	            	actual  : "40c07e69-dff1-41df-84c8-3f701c965577"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-740b2abd-b474-451d-b3e5-70f953f38ae5
        	            	+40c07e69-dff1-41df-84c8-3f701c965577
        	Test:       	TestGetEvaluationDistributions
    TestGetEvaluationDistributions: evaluation_test.go:178: 
        	Error Trace:	evaluation_test.go:178
        	Error:      	Not equal: 
        	            	expected: "bar"
        	            	actual  : "foo"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-bar
        	            	+foo
        	Test:       	TestGetEvaluationDistributions
--- FAIL: TestGetEvaluationDistributions (0.03s)
=== RUN   TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:262: 
        	Error Trace:	evaluation_test.go:262
        	Error:      	Not equal: 
        	            	expected: "e84b27ab-69a2-4f65-b5f9-baa33f95d97c"
        	            	actual  : "77b5e51a-e419-4eef-afc3-b09896b1e1a3"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-e84b27ab-69a2-4f65-b5f9-baa33f95d97c
        	            	+77b5e51a-e419-4eef-afc3-b09896b1e1a3
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:263: 
        	Error Trace:	evaluation_test.go:263
        	Error:      	Not equal: 
        	            	expected: "foo"
        	            	actual  : "bar"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-foo
        	            	+bar
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:264: 
        	Error Trace:	evaluation_test.go:264
        	Error:      	Not equal: 
        	            	expected: 80
        	            	actual  : 20
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:268: 
        	Error Trace:	evaluation_test.go:268
        	Error:      	Not equal: 
        	            	expected: "77b5e51a-e419-4eef-afc3-b09896b1e1a3"
        	            	actual  : "e84b27ab-69a2-4f65-b5f9-baa33f95d97c"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-77b5e51a-e419-4eef-afc3-b09896b1e1a3
        	            	+e84b27ab-69a2-4f65-b5f9-baa33f95d97c
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:269: 
        	Error Trace:	evaluation_test.go:269
        	Error:      	Not equal: 
        	            	expected: "bar"
        	            	actual  : "foo"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-bar
        	            	+foo
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:270: 
        	Error Trace:	evaluation_test.go:270
        	Error:      	Not equal: 
        	            	expected: 20
        	            	actual  : 80
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:301: 
        	Error Trace:	evaluation_test.go:301
        	Error:      	Not equal: 
        	            	expected: "e84b27ab-69a2-4f65-b5f9-baa33f95d97c"
        	            	actual  : "77b5e51a-e419-4eef-afc3-b09896b1e1a3"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-e84b27ab-69a2-4f65-b5f9-baa33f95d97c
        	            	+77b5e51a-e419-4eef-afc3-b09896b1e1a3
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:302: 
        	Error Trace:	evaluation_test.go:302
        	Error:      	Not equal: 
        	            	expected: "foo"
        	            	actual  : "bar"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-foo
        	            	+bar
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:303: 
        	Error Trace:	evaluation_test.go:303
        	Error:      	Not equal: 
        	            	expected: 80
        	            	actual  : 20
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:307: 
        	Error Trace:	evaluation_test.go:307
        	Error:      	Not equal: 
        	            	expected: "77b5e51a-e419-4eef-afc3-b09896b1e1a3"
        	            	actual  : "e84b27ab-69a2-4f65-b5f9-baa33f95d97c"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-77b5e51a-e419-4eef-afc3-b09896b1e1a3
        	            	+e84b27ab-69a2-4f65-b5f9-baa33f95d97c
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:308: 
        	Error Trace:	evaluation_test.go:308
        	Error:      	Not equal: 
        	            	expected: "bar"
        	            	actual  : "foo"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-bar
        	            	+foo
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
    TestGetEvaluationDistributions_MaintainOrder: evaluation_test.go:309: 
        	Error Trace:	evaluation_test.go:309
        	Error:      	Not equal: 
        	            	expected: 20
        	            	actual  : 80
        	Test:       	TestGetEvaluationDistributions_MaintainOrder
--- FAIL: TestGetEvaluationDistributions_MaintainOrder (0.04s)

To Reproduce

Checkout the mysql2 branch
Run mysql:latest in a container like:

➜ docker run -p 3306:3306 -e MYSQL_DATABASE=flipt_test -e MYSQL_USER=mysql -e MYSQL_PASSWORD=password -e MYSQL_ALLOW_EMPTY_PASSWORD=true mysql:latest

Run the tests a few times with DB_URL=mysql://mysql:password@localhost:3306/flipt_test make test
Note intermittent failures

Ideas

I'm assuming the issue has to do with inconsistent result ordering being return from the following line:
https://github.com/markphelps/flipt/blob/e8a5e2055340e6bcb194ed58b6fbd59ecc0af627/storage/db/common/evaluation.go#L101-L106

Which results in this SQL query:

SELECT d.id, d.rule_id, d.variant_id, d.rollout, v.`key` FROM distributions d JOIN variants v ON (d.variant_id = v.id) WHERE d.rule_id = ? ORDER BY d.created_at ASC

This seems like a pretty basic query.. so I'm not sure what I'm doing wrong? Again, this works fine in Postgres and SQLite, so im not sure if there is some MySQLism I am missing?

Here is the schema that I'm using for MySQL btw: https://github.com/markphelps/flipt/blob/mysql2/config/migrations/mysql/0_initial.up.sql

Any pointers would be greatly appreciated 🙇🏻

derekperkins · 2020-06-21T06:11:42Z

The generated query looks fine and should be reliable. I'm not quite sure what your schema is doing with the NOW(2) syntax for your defaults, and maybe that's causing it to not work as expected. Normally I'd expect to see something like this.

  `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
  `updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,

On an unrelated note, the only MySQLism that might get you is that pre-8.0 doesn't use true utf-8 by default. You should probably set your tables with DEFAULT CHARSET=utf8mb4 COLLATE= utf8mb4_general_ci to get real support.

sagikazarmark · 2020-06-21T15:29:13Z

I couldn't actually get the tests to run following the instructions above, because the schema migrations failed. After fixing them (#304), the tests are still failing though.

One possible reason for that is mysql timestamps store seconds and it looks like records are created too quickly (and the query orders records by the creation timestamp):

(Side note: this wouldn't be a problem if you used a lexographically sortable ID, like ULID or KSUID, because MySQL uses the primary key for ordering)

Another error I see is also related to time:

    TestUpdateRuleAndDistribution: rule_test.go:402:
        	Error Trace:	rule_test.go:402
        	Error:      	Not equal:
        	            	expected: 1592752733
        	            	actual  : 1592752734
        	Test:       	TestUpdateRuleAndDistribution

Generally, I think time is not a reliable way to order records. If order doesn't actually matter, then I would probably rewrite the tests to ensure each expected item can be found in the result set, regardless of the order.

Alternatively, you could mock out time and fake sleep between inserts. That requires you to manage creation and update timestamps, but that's usually in fact a good idea, to not rely on the database, but always set the timestamp from the code which guarantees you will always get the same result.

sagikazarmark · 2020-06-21T18:26:45Z

Further looking into the topic, it looks like mysql 8 actually supports storing fractional seconds with the original syntax I removed in #304, but my guess for the breaking tests is still wrong order because of time.

I guess you could add support for mysql 5.x and 8 separately, but I'm not sure it's worth it.

markphelps · 2020-06-24T15:11:53Z

@sagikazarmark @derekperkins Thanks for both of your help! I pushed a 'beta' version that supports MySQL to DockerHub:

docker pull markphelps/flipt:mysql

Please try it out if you can. I want to verify it works as expected before merging it into master and cutting a release version.

You can set it to connect to an existing MySQL DB by running it like so:

docker run -d \
    -p 8080:8080 \
    -p 9000:9000 \
    -e FLIPT_DB_URL=mysql://{user}:{password}@localhost:3306/{db} \
    markphelps/flipt:mysql

Here's more info about configuration and running with Docker.

markphelps added the enhancement label Feb 7, 2020

markphelps self-assigned this Feb 7, 2020

markphelps added the planned label Feb 26, 2020

markphelps added help wanted Halp wip Work In Progress and removed planned labels Jun 20, 2020

markphelps mentioned this issue Jun 24, 2020

Mysql2 #307

Merged

markphelps closed this as completed in #307 Jun 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Mysql #224

Support Mysql #224

markphelps commented Feb 7, 2020

derekperkins commented May 18, 2020

markphelps commented May 21, 2020

sagikazarmark commented Jun 3, 2020

markphelps commented Jun 4, 2020 •

edited

markphelps commented Jun 20, 2020 •

edited

derekperkins commented Jun 21, 2020

sagikazarmark commented Jun 21, 2020 •

edited

sagikazarmark commented Jun 21, 2020

markphelps commented Jun 24, 2020 •

edited

Support Mysql #224

Support Mysql #224

Comments

markphelps commented Feb 7, 2020

derekperkins commented May 18, 2020

markphelps commented May 21, 2020

sagikazarmark commented Jun 3, 2020

markphelps commented Jun 4, 2020 • edited

markphelps commented Jun 20, 2020 • edited

To Reproduce

Ideas

derekperkins commented Jun 21, 2020

sagikazarmark commented Jun 21, 2020 • edited

sagikazarmark commented Jun 21, 2020

markphelps commented Jun 24, 2020 • edited

markphelps commented Jun 4, 2020 •

edited

markphelps commented Jun 20, 2020 •

edited

sagikazarmark commented Jun 21, 2020 •

edited

markphelps commented Jun 24, 2020 •

edited