ptrdom commented on code in PR #537:
URL:
https://github.com/apache/pekko-persistence-r2dbc/pull/537#discussion_r4455227494
##########
core/src/main/scala/org/apache/pekko/persistence/r2dbc/ConnectionFactoryProvider.scala:
##########
@@ -197,10 +200,20 @@ class ConnectionFactoryProvider(system: ActorSystem[?])
extends Extension {
builder.option(PostgresqlConnectionFactoryProvider.SSL_PASSWORD,
settings.sslPassword)
}
- if (settings.driver == "mysql") {
- // Either `connectionTimeZone = SERVER` or
`forceConnectionTimeZoneToSession = true` need to be set for timezones to work
correctly,
- // likely caused by bug in
https://github.com/asyncer-io/r2dbc-mysql/pull/240.
- builder.option(Option.valueOf("connectionTimeZone"), "SERVER")
+ if (mySQL) {
+ // Instants are converted with the connection time zone, which must be
the session time zone for timestamps to be
+ // stored correctly, see
https://github.com/asyncer-io/r2dbc-mysql/pull/240. UTC has no ambiguous local
times when
+ // daylight saving time ends, and the server time zone may not be
resolvable by the driver (e.g. `EDT`), see
+ // https://github.com/apache/pekko-persistence-r2dbc/issues/533.
+ // Unlike the MariaDB time zone, these are not enforced on purpose: rows
written by earlier versions may be stored
+ // shifted, and the `url` or a customizer can keep the previous options
until the data is corrected, see the
+ // migration guide.
+ val connectionTimeZone = Option.valueOf[String]("connectionTimeZone")
+ val forceConnectionTimeZoneToSession =
Option.valueOf[java.lang.Boolean]("forceConnectionTimeZoneToSession")
+ if (!driverOptions.hasOption(connectionTimeZone))
+ builder.option(connectionTimeZone, "UTC")
+ if (!driverOptions.hasOption(forceConnectionTimeZoneToSession))
+ builder.option(forceConnectionTimeZoneToSession,
java.lang.Boolean.TRUE)
Review Comment:
Thanks, I added a warning. The plugin now logs one when
`forceConnectionTimeZoneToSession` isn't enabled and `connectionTimeZone` isn't
`SERVER`. It checks the final options, so it also covers a customizer. The
MySQL section of `getting-started.md` mentions it.
I didn't go with setting neither option, because the driver defaults
(`connectionTimeZone = LOCAL`, `forceConnectionTimeZoneToSession = false`)
wouldn't be safer:
- A `url` with only `forceConnectionTimeZoneToSession=false` would convert
with the JVM time zone while the session stays in the server time zone. That's
the original #533 problem, and it would still be silent.
- A `url` with only `connectionTimeZone=UTC` is correct today because the
plugin adds `forceConnectionTimeZoneToSession=true`. Setting neither would make
it store shifted timestamps.
`MySQLConnectionFactoryProviderSpec` now checks that the warning is logged
for the `url` in your example, and not for the `driver` setting, a plain `url`,
or a `url` with `connectionTimeZone=SERVER`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
