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

Core implementation for MySQL support #175

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

ptrdom
Copy link
Member

@ptrdom ptrdom commented Nov 12, 2024

Partially resolves #158.
Split off from #170.

@@ -101,7 +101,7 @@ pekko.persistence.r2dbc {
// #connection-settings
pekko.persistence.r2dbc {

# postgres or yugabyte
# postgres, yugabyte or mysql
Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure how to best deal with configuration differences for different databases, but suggesting alternatives on every setting does not seem very usable for users. Maybe something like https://github.com/lightbend/config?tab=readme-ov-file#inheritance?

Copy link
Member Author

Choose a reason for hiding this comment

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

By inheritance I mean defining groups of settings related to certain databases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any examples of other configs that have defaults in reference.conf that are not appropriate for mysql users? I'm not against adding mysql specific configs in some cases

Copy link
Member Author

Choose a reason for hiding this comment

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

Config differences between dialects are expressed here:

val dialectConfig = dialect match {
case "postgres" =>
ConfigFactory.parseString("""
pekko.persistence.r2dbc.connection-factory {
driver = "postgres"
host = "localhost"
port = 5432
user = "postgres"
password = "postgres"
database = "postgres"
}
""")
case "yugabyte" =>
ConfigFactory.parseString("""
pekko.persistence.r2dbc.connection-factory {
driver = "postgres"
host = "localhost"
port = 5433
user = "yugabyte"
password = "yugabyte"
database = "yugabyte"
}
""")
case "mysql" =>
ConfigFactory.parseString("""
pekko.persistence.r2dbc{
connection-factory {
driver = "mysql"
host = "localhost"
port = 3306
user = "root"
password = "root"
database = "mysql"
}
db-timestamp-monotonic-increasing = on
use-app-timestamp = on
}
""")
}

So different drivers, default ports, credentials, database names, and with MySQL we require following to be set:

db-timestamp-monotonic-increasing = on 
use-app-timestamp = on 

Journal will throw error upon initialization if these settings are in different values.

I will try coming up with something more ergonomic, of course you can provide suggestions if you have preferences.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at those, I appreciate the problem but I think in the short term, we could document the set up for the 3 dialects. We could do a follow up PR that makes tries to simplify the set up.

Copy link
Member Author

Choose a reason for hiding this comment

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

All good, I see current documentation already has info for Postgres and Yugabyte - https://pekko.apache.org/docs/pekko-persistence-r2dbc/current/connection-config.html,, so I will add a new tab for MySQL.

import pekko.persistence.r2dbc.internal.Sql.DialectInterpolation
import pekko.persistence.r2dbc.query.scaladsl.QueryDao

class MySQLQueryDao(
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add @since 1.1.0 on new public classes and methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, forgot to make these new classes private. I think they should be - if users really want to change journal's behavior they can always wire in their own, no need for us to maintain additional public API for that.

@ptrdom ptrdom requested a review from pjfanning November 13, 2024 17:47
@@ -0,0 +1,6 @@
DROP TABLE IF EXISTS event_journal;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the same license header from the create sql?

@@ -0,0 +1,13 @@
services:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a license header like the one on the create sql

@ptrdom ptrdom requested a review from pjfanning November 13, 2024 18:33
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - thanks

I'll leave open for a few days to allow others to review

@pjfanning pjfanning added this to the v1.1.0 milestone Nov 13, 2024
@pjfanning pjfanning merged commit 719e9eb into apache:main Nov 15, 2024
41 checks passed
@pjfanning
Copy link
Contributor

I'll merge to unblock projection work

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.

Support MySQL
2 participants