Skip to content
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.

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

Fix time zone issue in Rounding serialization #50845

Merged
merged 2 commits into from
Jan 13, 2020

Conversation

cbuescher
Copy link
Member

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 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.

Closes #50827

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.
@cbuescher cbuescher added :Analytics/Aggregations Aggregations >test-failure Triaged test failures from CI v7.6.0 labels Jan 10, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@nik9000
Copy link
Member

nik9000 commented Jan 10, 2020

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.

@cbuescher
Copy link
Member Author

caused all kinds of test failures in master

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.

@polyfractal
Copy link
Contributor

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:

ZoneId.of(timeZone, ZoneId.SHORT_IDS).getRules().equals(ZoneId.of(that.timeZone, ZoneId.SHORT_IDS).getRules()

Perhaps something like that is needed? E.g. compare the rules of the timezones rather than their pre/post-normalized IDs

which curiously has Zulu Time Zone id "Z"

Yeah, this drove me crazy dealing with it in Rollups too :(

@nik9000
Copy link
Member

nik9000 commented Jan 10, 2020

@cbuescher, your change makes the RoundingWireTests fail about half the time. I pushed a little patch that gets them passing. I think it should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >test-failure Triaged test failures from CI v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants