Support MySQL

[ptrdom]

Resolves #158.

I skipped the implementation of tags and migration tool, seemed not critical for the first release.

[ptrdom]

From the latest workflow run I can see two problems:

1. 15 second wait on receiveMessage probe in EventSourcedEndToEndSpec and EventSourcedChaosSpec is just a smidge too low - even local execution times for those specs are all over the place, and I have seen them fail from time to time, so I think it makes sense to increase the wait time for the relatively slow CI machines.
2. On JDK 8 specs that spawn multiple actors to persist messages fail with Too many events stored with the same timestamp [2024-11-03T17:47:17.954Z], buffer size [10], now that is a problem with changes to fractional units in java.time classes between JDKs - with multiple actors persisting messages at once it is quite easier to run into issues with only milli precision compared to micro. I am guessing Postgres/Yugabyte tests do not fail because of this because they do not use application timestamps. Not too sure about how to handle this, some of the options would be:
   1. Accept that these tests can be flaky on JDK 8.
   2. Not run these tests on JDK 8.
   3. Not test on JDK 8 at all. Even JDK 11 is EOL now, so we might as well just move on from them and the issues they cause.
   4. Bump the buffer size to not run into this issue on these tests.
   5. Try make sure that application generated timestamps always have 6 fractional unit precision. I initially preferred this option, but a nice solution to make this work was not obvious to me.

@pjfanning What do you think about this?

[pjfanning]

@ptrdom I haven't looked too closely at the test issues.

Would it be possible to compile the test code with all LTS Java versions including Java 8. I'm happy enough to not run the tests with Java 11. We can add docs that recommend that MySQL users use Java 11 as a minimum but that they could use Java 8 but we are not in a position to fully stand over that version.

[ptrdom]

I'm happy enough to not run the tests with Java 11.

@pjfanning I assume you meant that you are fine with not running tests on JDK 8 here, right? We can run on JDK 11 fine.

[pjfanning]

I'm happy enough to not run the tests with Java 11.

@pjfanning I assume you meant that you are fine with not running tests on JDK 8 here, right? We can run on JDK 11 fine.

Yes I meant, please compile the source and tests with Java 8 too and it's ok to skip the tests - but run the tests with all other Java LTS versions

[ptrdom]

@pjfanning What is next for this PR? Do I just wait for reviews, or is there something for me to do while I wait, like update docs?

[pjfanning]

I just merged #171 so you will need to fix the merge conflicts. Generally, the changes look good to me but with a large change, it would be nice for other reviews.

I only have my mobile right now so I can't see the full change but I vaguely recall that this relies on unreleased changes in core pekko modules. So we might need to release those before we merge this.

[ptrdom]

I only have my mobile right now so I can't see the full change but I vaguely recall that this relies on unreleased changes in core pekko modules. So we might need to release those before we merge this.

If you have the recent runtime config related PRs in mind, then this PR does not depend on them.

[pjfanning]

The values allowed are empty and "mysql" ? Could we list the allowed values in the comment?

[ptrdom]

No, valid dialect values are declared here - https://github.com/apache/pekko-persistence-r2dbc/blob/main/core/src/main/resources/reference.conf#L105. Setting dialect to empty enables loading of DAO classes specified by journalDaoClass, queryDaoClass, snapshotDaoClass, durableStateDaoClass configs instead of using default Postgres/Yugabyte implementation. This is the only way I figured MySQL implementation could be plugged in as a separate sbt module. This also provides flexibility in a way that library's users could plug their own DAO implementations and provide support for new databases without making changes to the library.

If you have any suggestions for improvements to implementation or docs, please let me know,

[pjfanning]

your link says postgre and yugabyte and the only valid values

I am lost. I do not understand what you are trying to do here.

[ptrdom]

I am not sure how else to explain it without you having to familiarize with the code more to either understand my solution or propose an alternative that is better.

[pjfanning]

we need to change the source header on all the new files - see point 5 in https://github.com/apache/pekko/blob/main/CONTRIBUTING.md#pull-request-requirements

[pjfanning]

apologies @ptrdom - can you remove your last header change commit?

These new classes are copies of old classes, so they must preserve the header of the old class. It is only absolutely new classes that need the standard ASF header.

[ptrdom]

Okay, I can revert. I do not understand the definition of "copy" and "absolutely new" in this context, so I will need some further explanation to avoid header issues in the future or just continued handholding is honestly fine.

[pjfanning]

• if you copy/paste code from another source file (whole class or just some lines), you need to also copy the source header from the file you copied from (even if you modify that copied code)
• if you add new code to a source file and that source file does not have a source header that matches the license under which your new code is licensed, then you should add the right source header (but don't remove any existing source headers)
• you can avoid duplication
  • you might have 2 Apache source headers that might have different copyright lines - you can merge those headers into one header but with 2 copyright lines
  • in Pekko, we use 2 forms of the Apache header - one that mentions Akka but we also use the standard Apache header if none of the code in the file is derived from Akka - we don't need both Apache headers in the same file - we just use the Pekko specific one (mentioning Akka)

[pjfanning]

Reviewing the new files on a case by case basis, it looks like some files should have the standard Apache header and some should have the special one that mentions Akka.
Maybe, we can get back to this just before we merge this. I can make the changes if the PR allows Pekko members to edit them (the default setting).

[ptrdom]

Yeah, I was about to comment on that, because most of changes are subclass implementations without any changes that would without a doubt could be called a copy/paste.

[pjfanning]

I am against the new reference.conf in its existing form.

I would suggest that dialect does not appear in the new reference.conf and that we change the projection-core reference.conf to mention that dialect=mysql is valid but that you also need to add a dependency to projection-mysql jarif you set that value.

The projection-mysql reference.conf should scope its configs with pekko.persistence.r2dbc.mysql instead of pekko.persistence.r2dbc.

[pjfanning]

please revert these import changes - imports start with 'pekko.' and we import 'org.apache.pekko' to allow that

[ptrdom]

I would suggest that dialect does not appear in the new reference.conf and that we change the projection-core reference.conf to mention that dialect=mysql is valid but that you also need to add a dependency to projection-mysql jarif you set that value.

The projection-mysql reference.conf should scope its configs with pekko.persistence.r2dbc.mysql instead of pekko.persistence.r2dbc.

I understand the reference.conf changes you are proposing, but I do not understand how they are supposed to work within the implementation. Could you provide examples of how your proposed config should work with MySQL implementation being in a separate sbt module? Mainly I do not see how to meaningfully implement dialect = mysql, what should core do when it encounters it, why are we making core aware of MySQL?

[ptrdom]

Mainly I do not see how to meaningfully implement dialect = mysql, what should core do when it encounters it, why are we making core aware of MySQL?

I am mentioning core here, because I assume your proposed changes apply both to core and projection reference.conf files, because their configs and resulting implementations are based on the same idea.

[pjfanning]

Mainly I do not see how to meaningfully implement dialect = mysql, what should core do when it encounters it, why are we making core aware of MySQL?

I am mentioning core here, because I assume your proposed changes apply both to core and projection reference.conf files, because their configs and resulting implementations are based on the same idea.

This PR is not working for me. It introduces too much. I don't see why we need so many new modules. I think we need a total of one new module - projection-mysql.

[ptrdom]

This PR is not working for me. It introduces too much. I don't see why we need so many new modules. I think we need a total of one new module - projection-mysql.

This project is not only about projections, it is also about journal implementation, that is why there is a split between core (journal) and projection modules.

If introducing new modules seems troublesome, then we could just stick everything into single module, just how it was done with Postgres and Yugabyte implementations.

[pjfanning]

This PR is not working for me. It introduces too much. I don't see why we need so many new modules. I think we need a total of one new module - projection-mysql.

This project is not only about projections, it is also about journal implementation, that is why there is a split between core (journal) and projection modules.

If introducing new modules seems troublesome, then we could just stick everything into single module, just how it was done with Postgres and Yugabyte implementations.

At this stage, that seems preferable to me. As long as the mysql jars are provided dependencies so that we don't force postgres users to load the mysql jar.

[pjfanning]

Is it possible that we could split this up?

Could we start by adding mysql support to pekko-projection-jdbc ? Just that module in 1st PR.

[pjfanning]

Is it possible that we could split this up?

Could we start by adding mysql support to pekko-projection-jdbc ? Just that module in 1st PR.

Ignore this - too many modules to worry about. Obviously this is an rdbc PR.

[ptrdom]

I do not understand the fixation on projections module only, because this project is about core (journal) and projection implementation, so any new database implementation in seperate sbt modules would mean two new modules to the project - use of journal does not imply the need to use projections, but projections without journal are realistically meaningless.

We can surely merge MySQL implementation back to core and projection modules, though I did ask for approval before working on separate modules and did spend additional effort to make separate modules work while keeping backwards compatibility on existing modules, so a pivot at this point certainly is not encouraging for me as a contributor. So can you confirm specific implementation parameters for me that would end up with a merged PR that has MySQL support?

[pjfanning]

I do not understand the fixation on projections module only, because this project is about core (journal) and projection implementation, so any new database implementation in seperate sbt modules would mean two new modules to the project - use of journal does not imply the need to use projections, but projections without journal are realistically meaningless.

We can surely merge MySQL implementation back to core and projection modules, though I did ask for approval before working on separate modules and did spend additional effort to make separate modules work while keeping backwards compatibility on existing modules, so a pivot at this point certainly is not encouraging for me as a contributor. So can you confirm specific implementation parameters for me that would end up with a merged PR that has MySQL support?

I appreciate that there has been a pivot. This is a large PR and had to review. There is a strong argument that the existing structure of this repo and its jars makes it hard to add support for extra DB implementations.

I have never used pekko-projections or akka-projections. I am just a volunteer. There are no other volunteers currently showing much interest in pekko-projections.

Now that I know more about the code, I think it is better to expand the existing modules than to introduce new ones. I would prefer if we start with core module (one PR) and later do a separate PR for the projection module (a 2nd PR).

If you want to discontinue work on this, that is understood.

[ptrdom]

We can take a mulligan on this one and proceed as you suggested. I will close this PR, because I feel the changes are significant enough to warrant that.

I definitely do sympathize with your position in this project, but I still feel that this situation was avoidable and such last minute rewrites are simply not sustainable, so I hope we can proceed forward with a healthy collaboration and make this situation an exception rather than a rule.