Support MySQL

[ptrdom]

Since MySQL is still one of the most popular RDMS, it makes a lot of sense to add support for it, hopefully helping with adoption of Pekko Persistence R2DBC.

Looking through the existing Postgres implementation for potential challenges with MySQL support, it seems to me that the main difference between the databases is that MySQL does not have concept of a transaction_timestamp. There are ways to implement it - for example wrapping a write operation in a procedure that has a local variable with a timestamp that gets reused in a transaction that is started, but I am not sure if there is much benefit in trying to implement database-side timestamps - I do not understand why it should be a dealbreaker for a production-ready implementation, because no matter where you get your timestamps from, whether it is the application or the database, you will still be dealing with potential clock skews, just that the risk levels might be different, dependent on the architecture of a particular deployment. So my thinking is that we could implement MySQL support by requiring the use of use-app-timestamp = on and possibly db-timestamp-monotonic-increasing = off. Could there be any issues with this approach?

Are there any more issues with the implementation or can we just proceed with it?

[pjfanning]

I think it would be good to do an experimental implementation where we document that the behaviour may change over a few releases. The initial version can use the simplest approaches to begin with and we can replace those if they prove to be problematic.

@ptrdom if you interested in taking this on, I'll be happy to review it. Presumably, https://github.com/asyncer-io/r2dbc-mysql will be used.

[ptrdom]

@pjfanning Yes, planning to pick this up soon. Your suggested approach makes total sense.

Would it be beneficial to have MySQL support in a separate repository? I remember how when working on endpoints4s we started breaking up the repository apart, putting different implementations in separate repositories to ease up the burden of maintaining every single implementation once we change something in the core. I guess that does not seem critical when number of implementations is small, but as the number grows it does become an increasing burden on maintainers. Although we could say that databases like Postgres and MySQL are the core of pekko-persistence-r2dbc that we do want to keep most up to date, so splitting to a separate repository would just complicate that.

Even if we stay in a single repository for this work, it would probably make sense to have a sbt module per implementation, not sticking support of every database into core sbt module, right?

[pjfanning]

sure - keep the mysql changes out of the core module - having its own module makes sense

[ptrdom]

Trying to figure out how to best hook up per database implementations with multiple sbt module approach. Since the only existing implementation has not created abstractions for this, we need to come up with a solution.

The way to do similar things to what we need in Pekko ecosystem repositories seems to be by specifying class name in config and constructing an instance of it by the use of reflection. To make this work I think that instead of this:

pekko-persistence-r2dbc/core/src/main/resources/reference.conf

Line 105 in 7dd8b64

dialect = "postgres"

We need to do something like dialect.class, which would allow providing different implementations of JournalDao:

pekko-persistence-r2dbc/core/src/main/scala/org/apache/pekko/persistence/r2dbc/journal/JournalDao.scala

Line 82 in 7dd8b64

private[r2dbc] class JournalDao(journalSettings: R2dbcSettings, connectionFactory: ConnectionFactory)(implicit

R2dbcJournal would then load the specified implementation. We would also convert current JournalDao to a trait and add a Postgres/Yugabyte implementation for it.

We could also start with splitting Postgres/Yugabyte implementation to its own module and get the abstractions right before proceeding with MySQL support. Good idea with this work would be to also make test suites as traits that are implemented by each database implementation separately.

@pjfanning Any thoughts? I am up to do any solution for this and do not have any strong opinions.

[pjfanning]

I don't want to break backward compatibility so despite having postgres in projection-core seeming like a bad idea. I'd prefer to leave it there, at least for now.

[ptrdom]

Aha, well I guess the solution can be contorted to remain backwards compatible. If dialect.class is okay, then the config can be added just for MySQL and R2dbcJournal would fallback to current dialect config if dialect.class is not provided. JournalDao is a class than can just be overriden by MySQL implementation. And of course the tests can be restructured to work both for Postgres and MySQL.

[ptrdom]

@pjfanning Since you mentioned need to not break backwards compatibility, I started thinking about binary compatibility and noticed that MiMa plugin is not setup for this repo. Would it make sense to set it up?

[pjfanning]

@ptrdom it would be good to get the mima checks set up

[ptrdom]

@pjfanning There is an issue with keeping backwards compatibility while implementing projection support for MySQL - R2dbcProjectionSettings. This class needs a new attribute for dialect, and since it is a case class there is no way to not break it. Is it okay to break backwards compatibility and convert this to a class, or do you see a different way to proceed?

[pjfanning]

@pjfanning There is an issue with keeping backwards compatibility while implementing projection support for MySQL - R2dbcProjectionSettings. This class needs a new attribute for dialect, and since it is a case class there is no way to not break it. Is it okay to break backwards compatibility and convert this to a class, or do you see a different way to proceed?

Could you add a 2nd constructor to the case class that has the legacy param list and that calls the changed main constructor, setting a sensible default on the new params?

[ptrdom]

Could you add a 2nd constructor to the case class that has the legacy param list and that calls the changed main constructor, setting a sensible default on the new params?

That still throws binary compatibility errors, mentioning copy, unapply and this. case class is just cursed when used for public APIs 😞

[pjfanning]

Could you add a 2nd constructor to the case class that has the legacy param list and that calls the changed main constructor, setting a sensible default on the new params?

That still throws binary compatibility errors, mentioning copy, unapply and this. case class is just cursed when used for public APIs 😞

I understand and we can add an excludes file with Problem filters. Having the 2nd constructor means that source compatibility will be preserved for the constructor calls.

[ptrdom]

Sure, we can do that.

The sensible default for dialect would be to pick any one from Postgres and Yugabyte, because there is no difference between them now.