feat: support for r2dbc sqlserver #1122

sebastian-alfers · 2024-02-04T15:19:04Z

Still depends on a snapshot version, so do not merge.

patriknw

looking good

project/Dependencies.scala

build.sbt

patriknw · 2024-02-05T14:06:34Z

...projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/SqlServerOffsetStoreDao.scala

+        .mkString(",")
+      sql"""
+      $base
+      AND CONCAT(persistence_id, '-', seq_nr) NOT IN ($listParam)"""


not possible to use a bind parameter for that $listParam? such things could be source of sql injection

I don't think so, but I can have another look.

No, does not seem it works:

Cause: java.lang.IllegalArgumentException: Cannot encode [[Ljava.lang.String;@5b565160] parameter of type [[Ljava.lang.String;]

The currently supported codecs can be found here representing this list of java types. The maintainer does mention this as well here.

I am trying now to transform each item within notInLatestBySlice to its own param to end up with ... NOT IN(?,?,?,...) and then one single .bind() per element.

I my last commit I removed the pending case for sqlserver. The test is still flaky, but at least it went green now to indicate this change ⬆️ works. It if keeps being flaky, we can re-add the pending exclusion, but not we at least know it works.

Good that you found a way to use parameters.

Now the concat of the pid and seqNr in notInLatestBySlice is done in the OffsetStore, and then the dao is assumed to use the same format. We can make a more clear interface by adding a:

case class LatestBySlice(slice: Int, pid: String, seqNr: Long)

and the dao method would be

def deleteOldTimestampOffsetSql(notInLatestBySlice: Seq[LatestBySlice])

The main reason why I started thinking about that was that it might be good to use fixed number of parameters in the sql. Otherwise we can end up with very many different prepared statements.

That fixed number of parameters would then be the same as number of slices in the slice range. For the slices that are not included in the notInLatestBySlice we could still bind a fake value (such as "-") to the parameter.

If I do understand you right, you suggest one param per (timestampOffsetBySlicesSourceProvider.minSlice to timestampOffsetBySlicesSourceProvider.maxSlice). Is that right? Then there is a limit in the server:

io.r2dbc.mssql.ExceptionFactory$MssqlNonTransientException: The incoming request has too many parameters. The server supports a maximum of 2100 parameters. Reduce the number of parameters and resend the request. at io.r2dbc.mssql.ExceptionFactory.createException(ExceptionFactory.java:154)

Maybe I can fix this, let me see.

That is right, should be at most 1024 parameters because we have at most 1024 slices for one Projection instance (for one R2dbcOffsetStore).

I pushed my take on how I understood your suggestion.

patriknw

good, just something minor

akka-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/R2dbcOffsetStore.scala

...projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/SqlServerOffsetStoreDao.scala

project/Dependencies.scala

patriknw

looking good, only minor comments, and the docs should also be updated https://doc.akka.io/docs/akka-projection/current/r2dbc.html

patriknw · 2024-03-14T10:03:00Z

...-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/PostgresOffsetStoreDao.scala

@@ -42,13 +53,16 @@ private[projection] class PostgresOffsetStoreDao(
    projectionId: ProjectionId)
    extends OffsetStoreDao {

-  private val logger = LoggerFactory.getLogger(getClass)
+  protected def logger: Logger = LoggerFactory.getLogger(getClass)


could you do the same as in a-p-r2dbc to avoid accessing LoggerFactory each time:

protected def log: Logger = PostgresOffsetStoreDao.log

should PostgresOffsetStoreDao.log then be a val, or what is the purpose?

Yes, so that it's created once instead of for each log.x call

Ok, makes sense. Then I think I made it wrong there as well: https://github.com/akka/akka-persistence-r2dbc/blob/main/core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerDurableStateDao.scala#L33. Will fix (if possible).

...-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/PostgresOffsetStoreDao.scala

akka-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/R2dbcOffsetStore.scala

…ernal/PostgresOffsetStoreDao.scala Co-authored-by: Patrik Nordwall <[email protected]>

…ernal/R2dbcOffsetStore.scala Co-authored-by: Patrik Nordwall <[email protected]>

patriknw · 2024-03-20T10:47:06Z

...-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/PostgresOffsetStoreDao.scala

+ */
+@InternalApi
+private[projection] object PostgresOffsetStoreDao {
+  private def log: Logger = LoggerFactory.getLogger(classOf[PostgresOffsetStoreDao])


This should be a val

patriknw · 2024-03-20T10:51:36Z

...-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/PostgresOffsetStoreDao.scala

@@ -42,13 +61,16 @@ private[projection] class PostgresOffsetStoreDao(
    projectionId: ProjectionId)
    extends OffsetStoreDao {

-  private val logger = LoggerFactory.getLogger(getClass)
+  protected def logger: Logger = PostgresOffsetStoreDao.log


why do we need to change this from val to def in the first place? doesn't getLogger(getClass) do exactly what we want? if that is the case we don't need the extra val in the companion object

IIRC, in a-p-re2dc, I had to add it to avoid some conflicts caused by inheritance. Removing it here, maybe it was needed at some point, but is not anymore.

patriknw

looking good, only some docs and then we can merge

sebastian-alfers · 2024-03-22T18:06:07Z

docs/src/main/paradox/r2dbc.md

@@ -57,6 +57,12 @@ The table below shows `akka-projection-r2dbc`'s direct dependencies, and the sec
 The `akka_projection_offset_store`, `akka_projection_timestamp_offset_store` and `akka_projection_management` tables
 need to be created in the configured database:

+@@@ warning


@patriknw As it is now, this warning is shown regardless of the chosen dialect tab.

So, in a-p-r2dbc, we took advantage of the paradoxGroups which allowed to make this a condition.

To get this warning only render if "SQLServer" tab is checked, we would also have to introduce paradoxGroups in the build.sbt, however this would show in all pages (that have no reference to those dialects). Any idea? I tried to hide the html-rendering of the dialect drop-down in the top-left via JS, and it worked, but it was not perfect.

That's fine, let's show the warning for all dialects here.

patriknw

LGTM, thanks

sebastian-alfers marked this pull request as draft February 4, 2024 16:32

patriknw reviewed Feb 5, 2024

View reviewed changes

patriknw reviewed Feb 9, 2024

View reviewed changes

akka-projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/R2dbcOffsetStore.scala Outdated Show resolved Hide resolved

...projection-r2dbc/src/main/scala/akka/projection/r2dbc/internal/SqlServerOffsetStoreDao.scala Outdated Show resolved Hide resolved

patriknw reviewed Feb 16, 2024

View reviewed changes

project/Dependencies.scala Outdated Show resolved Hide resolved

sebastian-alfers force-pushed the feat-sqlserver branch from 0db45c5 to 4144c2b Compare February 16, 2024 16:31

sebastian-alfers marked this pull request as ready for review February 27, 2024 10:26

ennru requested a review from patriknw February 27, 2024 11:09

sebastian-alfers force-pushed the feat-sqlserver branch from 4144c2b to 9948b0f Compare March 14, 2024 08:07

feat: sqlserver support

168f7ae

sebastian-alfers force-pushed the feat-sqlserver branch from 9948b0f to 168f7ae Compare March 14, 2024 08:22

patriknw reviewed Mar 14, 2024

View reviewed changes

sebastian-alfers and others added 3 commits March 15, 2024 18:55

Update akka-projection-r2dbc/src/main/scala/akka/projection/r2dbc/int…

a97c274

…ernal/PostgresOffsetStoreDao.scala Co-authored-by: Patrik Nordwall <[email protected]>

Update akka-projection-r2dbc/src/main/scala/akka/projection/r2dbc/int…

7e26916

…ernal/R2dbcOffsetStore.scala Co-authored-by: Patrik Nordwall <[email protected]>

comments from pr

3b5a9dd

ennru requested a review from patriknw March 20, 2024 10:44

patriknw reviewed Mar 20, 2024

View reviewed changes

removing PostgresOffsetStoreDao.log again

94ab7c8

sebastian-alfers mentioned this pull request Mar 20, 2024

fix: use val for logging in all SqlServer daos akka/akka-persistence-r2dbc#534

Merged

removed import;

fd7f92f

patriknw reviewed Mar 21, 2024

View reviewed changes

docs

a27d1bd

sebastian-alfers commented Mar 22, 2024

View reviewed changes

patriknw approved these changes Mar 25, 2024

View reviewed changes

Merge branch 'main' into feat-sqlserver

63b0393

johanandren merged commit 764bc38 into akka:main May 6, 2024
22 checks passed

johanandren added this to the 1.5.4 milestone May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for r2dbc sqlserver #1122

feat: support for r2dbc sqlserver #1122

sebastian-alfers commented Feb 4, 2024

patriknw left a comment

patriknw Feb 5, 2024

sebastian-alfers Feb 6, 2024

sebastian-alfers Feb 8, 2024

sebastian-alfers Feb 8, 2024

patriknw Feb 8, 2024

sebastian-alfers Feb 8, 2024

sebastian-alfers Feb 8, 2024

patriknw Feb 9, 2024

sebastian-alfers Feb 9, 2024

patriknw left a comment

patriknw left a comment

patriknw Mar 14, 2024

sebastian-alfers Mar 15, 2024

patriknw Mar 20, 2024

sebastian-alfers Mar 20, 2024

patriknw Mar 20, 2024

patriknw Mar 20, 2024

sebastian-alfers Mar 20, 2024

patriknw left a comment

sebastian-alfers Mar 22, 2024

patriknw Mar 25, 2024

patriknw left a comment

feat: support for r2dbc sqlserver #1122

feat: support for r2dbc sqlserver #1122

Conversation

sebastian-alfers commented Feb 4, 2024

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw left a comment

Choose a reason for hiding this comment