ptrdom commented on code in PR #537:
URL:
https://github.com/apache/pekko-persistence-r2dbc/pull/537#discussion_r4431115213
##########
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:
Added a "MySQL time zone in 2.0.0" section to `migration-guides.md`. It
covers:
- How to tell whether you're affected: the driver's `The server timezone is
unknown <…>, trying to use system default timezone` warning, together with a
JVM zone that differs from the server zone. It also gives a quick check,
`SELECT MAX(db_timestamp), UTC_TIMESTAMP(6) FROM event_journal` in a UTC
session.
- Why it matters: the `eventsBySlices` / projection offset skip you
described.
- Option 1: correct the data before upgrading, with all writers and
projections stopped. In a UTC session, it runs `CONVERT_TZ(CONVERT_TZ(col,
'+00:00', 'SYSTEM'), '<JVM zone>', '+00:00')` on `event_journal.db_timestamp`,
`durable_state.db_timestamp` and
`projection_timestamp_offset_store.timestamp_offset` / `timestamp_consumed`.
- Option 2: keep the previous options with a
`connection-factory-options-customizer` until the data is corrected. The guide
includes an example.
I checked both on MySQL 9.5.0 with the server in `America/New_York`:
- The correction SQL, run on the persistence and projection MySQL DDL. Rows
were written the way earlier versions wrote them with a JVM in
`Europe/Vilnius`, so they were stored 7h ahead. After the update, they hold the
exact instants.
- The customizer example, compiled as written. It puts the session back on
the server zone.
##########
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:
I'm keeping the escape hatch on purpose. It's the second option in the
migration guide for the shifted-data case, which `requireUtcTimezone`-style
enforcement would rule out. This is now explicit in both places:
- A comment at the option setting explains why, unlike MariaDB, these
options are set before the customizer.
- The new MySQL section in `getting-started.md` says that a customizer can
override them, that timestamps are then only correct if the connection time
zone is also the session time zone and has no daylight saving time, and that
this is meant for upgrades. It links to the migration guide.
##########
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:
- I moved the MySQL note to a new MySQL section in `getting-started.md`,
next to the MariaDB one. `config.md` now links to both sections.
- The migration guide entry now opens with the `NOW()` / `CURRENT_TIMESTAMP`
/ `TIMESTAMP` change for SQL in a `ChangeHandler`.
--
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]
