pjfanning commented on code in PR #537:
URL:
https://github.com/apache/pekko-persistence-r2dbc/pull/537#discussion_r4428242992
##########
core/src/main/scala/org/apache/pekko/persistence/r2dbc/ConnectionFactoryProvider.scala:
##########
@@ -198,9 +198,12 @@ class ConnectionFactoryProvider(system: ActorSystem[?])
extends Extension {
}
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")
+ // 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.
+ builder.option(Option.valueOf("connectionTimeZone"), "UTC")
Review Comment:
Upgrade hazard for the case this PR fixes. When the driver couldn't resolve
`@@system_time_zone` (`ZoneId.of(id, SHORT_IDS)` knows `EST` but not `EDT` or
`CEST`), it fell back to the JVM zone. If the JVM zone differs from the server
zone, every row written by released versions is stored shifted by that offset,
and round trips hid it.
After upgrading, old rows still read back shifted, but new rows are correct.
With the JVM ahead of the server (e.g. JVM in UTC, server in
`America/New_York`), old `db_timestamp`s and stored projection timestamp
offsets are about 4h in the future. New events then land below the offsets, and
`eventsBySlices` (`db_timestamp >= offset`) can silently skip events written in
roughly the first 4h after the upgrade. Backtracking won't cover a gap that
large.
Could you add a `migration-guides.md` entry that explains how to tell
whether you're affected (the driver logs `The server timezone is unknown <…>`,
and the JVM zone differs from the server zone) and what to do, e.g. keep the
old option with a `connection-factory-options-customizer`, or correct the data
before upgrading?
##########
core/src/main/scala/org/apache/pekko/persistence/r2dbc/ConnectionFactoryProvider.scala:
##########
@@ -198,9 +198,12 @@ class ConnectionFactoryProvider(system: ActorSystem[?])
extends Extension {
}
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")
+ // 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.
+ builder.option(Option.valueOf("connectionTimeZone"), "UTC")
+ builder.option(Option.valueOf("forceConnectionTimeZoneToSession"),
java.lang.Boolean.TRUE)
Review Comment:
For MariaDB, `MariaDBConnections.requireUtcTimezone` makes the plugin fail
to start if the url or a customizer sets a non-UTC `timezone`. For MySQL, a
customizer can still silently override `connectionTimeZone` /
`forceConnectionTimeZoneToSession`. Keeping that escape hatch is defensible,
especially given the upgrade case above, but it would be good to make the
choice explicitly and document it.
##########
docs/src/main/paradox/config.md:
##########
@@ -23,6 +23,12 @@ H2:
For H2 the connection is configured in the `h2` section of the connection
factory configuration, see
@ref:[H2](getting-started.md#h2). For MariaDB, see
@ref:[MariaDB](getting-started.md#mariadb).
+For MySQL the plugin sets the `connectionTimeZone` option of the driver to
`UTC` and `forceConnectionTimeZoneToSession`
+to `true`, so that timestamps are stored and read correctly whatever the time
zone of the server is, including the
+local hour that is repeated when daylight saving time ends. The database
session therefore uses the UTC time zone, and
+SQL that runs on the plugin's connections, for example in a `ChangeHandler` of
durable state, sees `NOW()` and
+`TIMESTAMP` values in UTC.
Review Comment:
Two docs nits:
- The MariaDB UTC session note is in `getting-started.md` (MariaDB section),
while this one is in `config.md`. It would be easier to find if both were in
the same place.
- The change in what `NOW()` / `CURRENT_TIMESTAMP` return in `ChangeHandler`
SQL is a behaviour change for existing users, so it's worth repeating in the
migration guide entry as well.
--
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]
