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

Add MySQL compatibility #80

Merged
merged 16 commits into from
Aug 13, 2019
Merged

Add MySQL compatibility #80

merged 16 commits into from
Aug 13, 2019

Conversation

BlackbitDevs
Copy link

@BlackbitDevs BlackbitDevs commented May 15, 2018

This PR provides JSON support to store activity data instead of MariaDB's dynamic columns. Whether MySQL or MariaDB shall be used can be configured in services.yml (change CustomerManagementFrameworkBundle\ActivityStore\ActivityStoreInterface to CustomerManagementFrameworkBundle\ActivityStore\MySQL).
For compatibility reasons it does not use JSON column type but keeps database structure untouched.

Solves #58

@markus-moser
Copy link
Contributor

Great, thank you!

We will take a look at it in detail.

One question: if we do not change the database structure the mysql JSON search/filter possibilities will not work, correct? Wouldn't it be better to create a new/separate table for the mysql store which supports the JSON datatype?

@BlackbitDevs
Copy link
Author

I am not sure which functionality you exactly mean. Could you point me to the code in the current master branch? Then I could look for the pendant in this PR (or say if it is missing).

@markus-moser
Copy link
Contributor

This functionality is not directly used within the bundle itself but could be used for example by segment builders (or other tools) to filter the activities table based on attributes within the "attributes" column.

For example:

WHERE COLUMN_GET(attributes, 'myAttribute' AS CHAR) = 'value xyz';

The JSON datatype supports similiar functionallities:
https://dev.mysql.com/doc/refman/5.7/en/json-search-functions.html

But I think this will only work if it is actually a JSON field.

@BlackbitDevs
Copy link
Author

Yes, with the current database structure you can use CAST(column AS JSON) (see https://dev.mysql.com/doc/refman/8.0/en/json.html#json-converting-between-types) and then you are able to filter. Of course this would not be the best solution regarding performance but we have to decide which minimum MySQL version we want to support. If 5.7 is acceptable then it certainly makes sense to use a separate datastore for MySQL.

@fashxp
Copy link
Member

fashxp commented May 15, 2018

from my point of view 5.7 would be fine ... since aws aurora also supports 5.7

@CLAassistant
Copy link

CLAassistant commented Sep 10, 2018

CLA assistant check
All committers have signed the CLA.

@Cruiser13
Copy link
Contributor

@fashxp any chance to merge this so we can use MySQL instead of MariaDB with the customer data framework?

@fashxp
Copy link
Member

fashxp commented Apr 4, 2019

we discussed our next steps internally.
we're going to merge this PR in order to support mysql < 5.7
in addition we would like to create an additional activity store that supports the JSON data type of mysql (since now both, mysql and mariadb support this one).
see also #105

@fashxp
Copy link
Member

fashxp commented Apr 4, 2019

@BlackbitNeueMedien are you done and have you tested the mysql store? did you also regression-test the mariadb store?

@BlackbitDevs
Copy link
Author

BlackbitDevs commented Apr 4, 2019

Yes, we run this fork on 2 productive projects and have not noticed any problems.
But we did not compare its behaviour to the mariadb version (because we currently do not use mariadb anywhere).

@Cruiser13
Copy link
Contributor

@fashxp can this be merged with the next cmf release?

@fashxp fashxp self-assigned this Aug 13, 2019
@fashxp fashxp added this to the v2.1.1 milestone Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants