Fix time zone issue in Rounding serialization#50845
Conversation
When deserializing time zones in the Rounding classes we used to include a tiny normalization step via `DateUtils.of(in.readString())` that was lost in elastic#50609. Its at least necessary for some tests, e.g. the cause of elastic#50827 is that when sending the default time zone ZoneOffset.UTC on a stream pre 7.0 we convert it to a "UTC" string id via `DateUtils.zoneIdToDateTimeZone`. This gets then read back as a UTC ZoneRegion, which should behave the same but fails the equality tests in our serialization tests. Reverting to the previous behaviour with an additional normalization step on 7.x.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
I started with this code and it caused all kinds of test failures in master. I'm curious why it doesn't fail in 7.x. I'll dig more when I'm properly awake. |
Interesting, I didn't check the situation on master, since there's no version forking in the serialization code necessary there. Better to do some more digging then to be sure. The whole thing isn't really an issue in real life because ZoneOffset.UTC (which curiously has Zulu Time Zone id "Z") and UTC should behave the same, but the test code barfs on equality comparison. |
|
Drive-by comment: rollup had a lot of issues with timezones (we accepted a string and didn't normalize it originally, then java-time happened, so we have a lot of deprecated stuff to deal with). Some of our equality checks deal with timezones like so: Perhaps something like that is needed? E.g. compare the rules of the timezones rather than their pre/post-normalized IDs
Yeah, this drove me crazy dealing with it in Rollups too :( |
|
@cbuescher, your change makes the |
When deserializing time zones in the Rounding classes we used to include a tiny
normalization step via
DateUtils.of(in.readString())that was lost in #50609.Its at least necessary for some tests, e.g. the cause of #50827 is that when
sending the default time zone ZoneOffset.UTC on a stream pre 7.0 we convert it
to a "UTC" string id via
DateUtils.zoneIdToDateTimeZone. This gets then readback as a UTC ZoneRegion, which should behave the same but fails the equality
tests in our serialization tests. Reverting to the previous behaviour with an
additional normalization step on 7.x.
Closes #50827