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.

Sign up for GitHub

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

Jump to bottom

feat: sqlserver / mssql support #505

Merged

patriknw merged 21 commits into akka:main from sebastian-alfers:mssql-support-2nd

Feb 2, 2024

Contributor

sebastian-alfers commented Jan 22, 2024 •

edited

Loading

Next iteration based on discussion in #494.

Note that in the sqlserver specific overrides, named binding is still used. This is recommended by the plugin, and my have several advantages:

no need to bind a param multiple times if needed,
no need to use the getAndIncIndex approach for conditional binding,
no need to bind in order and
more readable.

However, I see this is not aligned with the rest of the project, so I have no problem with converting to positional binding for where it is not yet used.

More notes:

the test fails when changing the log level of the root logger and / or of the sqlserver QUERY class to DEBUG
perf spec sometimes fails

sebastian-alfers marked this pull request as draft

January 22, 2024 10:29

patriknw reviewed

View reviewed changes

Member

patriknw left a comment

looking good

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TimestampCodec.scala Outdated


		override def encode(timestamp: Instant): LocalDateTime = LocalDateTime.ofInstant(timestamp, zone)

		override def now[T](): T = LocalDateTime.ofInstant(instantNow(), zone).asInstanceOf[T]

Member

patriknw Jan 22, 2024

that cast to T looks dangerous, for example this usage:

override def currentDbTimestamp(): Future[Instant] = Future.successful(timestampCodec.now())

wouldn't that cast a LocalDateTime to an Instant?

Contributor Author

sebastian-alfers Jan 22, 2024

Yes, right. I am fixing this now. It seems I can get rid of def now[T](): T completely, and fix the places where I am not yet binding a timestamp with the codec specific .bindTimestamp.

The unsafe casts must not yet have been triggered I assume.

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TimestampCodec.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/postgres/PostgresQueryDao.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/postgres/PostgresQueryDao.scala Outdated Show resolved Hide resolved

patriknw reviewed

View reviewed changes

Member

patriknw left a comment

looking good, a few comments about the timestamps

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TimestampCodec.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TimestampCodec.scala


		override def decode(row: Row, name: String): Instant = toInstant(row.get(name, classOf[LocalDateTime]))

		override def encode(timestamp: Instant): LocalDateTime = LocalDateTime.ofInstant(timestamp, zone)

Member

patriknw Jan 24, 2024

this is good, it wouldn't surprise me if r2dbc-sqlserver would add support for binding Instant in a future version. seems like a missing thing to me. anyway, for us this is good now

Contributor Author

sebastian-alfers Jan 24, 2024

They discussed it already here and here.

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TimestampCodec.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerQueryDao.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerQueryDao.scala Outdated Show resolved Hide resolved

johanandren reviewed

View reviewed changes

Member

johanandren left a comment

Looking good, left some questions and nitpicks but I think the only big thing missing is documentation.

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TagsCodec.scala Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/codec/TimestampCodec.scala Outdated Show resolved Hide resolved

migration-tests/src/test/scala/akka/persistence/r2dbc/migration/MigrationToolSpec.scala Outdated

		@@ -74,6 +74,7 @@ class MigrationToolSpec

		private val migration = new MigrationTool(system)

		// enable for sqlserver as well?

Member

johanandren Jan 26, 2024

Seems fine if it passes, if not, not sure it is important to deal with right away.

Contributor Author

sebastian-alfers Jan 27, 2024

Table creation would need to be tailored for sqlserver, skipping it for now.

Contributor Author

sebastian-alfers Jan 31, 2024

Should I create a ticket for this?

Member

patriknw Jan 31, 2024

The migration tool is only using methods in the dao that we are using for other things already, if I'm not misremembering. Therefore it is not that important to test it with all dialects. I think the comment can be removed and nothing more.

Member

patriknw Jan 31, 2024

@sebastian-alfers I think I misunderstood. The question wasn't only about the test. The MigrationTool has it's own MigrationDao, which would be different for sqlserver. Please create an issue and we handle it if it comes up as a request.

Contributor Author

sebastian-alfers Jan 31, 2024

docker/docker-compose-sqlserver.yml Show resolved Hide resolved

ddl-scripts/create_tables_sqlserver.sql Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerDurableStateDao.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerJournalDao.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerQueryDao.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/PayloadCodec.scala Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/SqlServerSnapshotDao.scala Outdated Show resolved Hide resolved

johanandren approved these changes

View reviewed changes

Member

johanandren left a comment •

edited

Loading

LGTM with a few more nitpicks and CI green. Would be good with a second LGTM before merging though.

core/src/main/scala/akka/persistence/r2dbc/internal/postgres/PostgresDurableStateDao.scala Outdated Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/postgres/PostgresQueryDao.scala Outdated Show resolved Hide resolved

patriknw reviewed

View reviewed changes

Member

patriknw left a comment

Looking good, only a few minor things.
Maybe it can be worth looking at the dialect in Akka Projections before we release to see that this fits in with that.

core/src/main/scala/akka/persistence/r2dbc/R2dbcSettings.scala Show resolved Hide resolved

core/src/main/scala/akka/persistence/r2dbc/internal/codec/QueryAdapter.scala Outdated


		package akka.persistence.r2dbc.internal.codec

		trait QueryAdapter {

Member

patriknw Jan 29, 2024

private[akka] on all these internal classes, and the usual InternalApi markers

Member

patriknw Jan 29, 2024

Maybe some of these will be needed for the sqlserver dialect in Akka Projections?

Contributor Author

sebastian-alfers Jan 29, 2024

I can look into projections regarding sqlserver.

Contributor Author

sebastian-alfers Jan 31, 2024

I was building the sqlserver dialect in projections based on a publishLocal snapshot from this branch, and did not see any problems in this regards.

ddl-scripts/create_tables_sqlserver.sql Outdated Show resolved Hide resolved

docs/src/main/paradox/getting-started.md Show resolved Hide resolved

docs/src/main/paradox/getting-started.md Outdated Show resolved Hide resolved

Contributor Author

sebastian-alfers commented Jan 31, 2024

Maybe it can be worth looking at the dialect in Akka Projections before we release to see that this fits in with that.

I looked at projections and did not see anything missing, when it comes to integrating changes from this PR.

patriknw marked this pull request as ready for review

January 31, 2024 10:39

patriknw approved these changes

View reviewed changes

Member

patriknw left a comment

LGTM

sebastian-alfers mentioned this pull request

Enable migrations for sqlserver dialect #509

Closed

sebastian-alfers and others added 20 commits

February 1, 2024 13:51


          feat: sqlserver / mssql support

c3fc764


          mima h2

c8cc056


          remove default QueryAdapter to make scala3 happy

75caebc


          unit test for sqlserver query adapter

b4d5298


          comments from pr

3187add


          comments from pr

909495a


          remove comment

5a609e7


          comments from pr

6f5e7d2


          Update core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/…

76019c2

…SqlServerDurableStateDao.scala

Co-authored-by: Johan Andrén <[email protected]>


          Update core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/…

e7c2a21

…SqlServerJournalDao.scala

Co-authored-by: Johan Andrén <[email protected]>


          Update core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/…

062ff69

…SqlServerSnapshotDao.scala

Co-authored-by: Johan Andrén <[email protected]>


          Update core/src/main/scala/akka/persistence/r2dbc/internal/sqlserver/…

b4c5a9d

…SqlServerQueryDao.scala

Co-authored-by: Johan Andrén <[email protected]>


          comments

f603057


          mima after moving PayloadCodec

bb35f8c


          docs

8c3f374


          remove comment

75c9de5

fmt

34e0904


          comments from pr

ef1a7e3


          timestampCoded protected

2ae1ddf


          make more codec instances private

86e8251

sebastian-alfers force-pushed the mssql-support-2nd branch from 6410fc8 to 86e8251 Compare

February 1, 2024 12:52


          mark mssql as experimental/unstable, disable perf specs

6a50a9b

patriknw merged commit e28e821 into akka:main

8 checks passed

patriknw added this to the 1.2.2 milestone

patriknw mentioned this pull request

chore: Make Sql.Interpolation backwards compatible #514

Merged

sebastian-alfers mentioned this pull request

docs: add missing getting-started docs for SQLServer #542

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet