Skip to content

[CI] InternalAggregationsTests testSerialization reproducibly failing #50827

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.

Sign up for GitHub

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

Jump to bottom
Closed
mark-vieira opened this issue Jan 9, 2020 · 3 comments
Closed

[CI] InternalAggregationsTests testSerialization reproducibly failing #50827

mark-vieira opened this issue Jan 9, 2020 · 3 comments
Assignees
@cbuescher
Labels
:Search/Search Search-related issues that do not fall into other categories >test-failure Triaged test failures from CI

Comments

@mark-vieira
Copy link
Contributor

org.elasticsearch.search.aggregations.InternalAggregationsTests » testSerialization has failed 7 times today and hasn't failed otherwise in the past month, so it's likely something is busted here.

Here's a build failure for which the reproduction line reproduces locally for me: https://gradle-enterprise.elastic.co/s/jrqqlb4u4unv2/tests/nfxodyych4uas-dbj4whiivduzy
@mark-vieira mark-vieira added :Search/Search Search-related issues that do not fall into other categories >test-failure Triaged test failures from CI labels Jan 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@mark-vieira mark-vieira changed the title [CI] InternalAggregationsTests.tesSerialization failing often [CI] InternalAggregationsTests.tesSerialization reproducibly failing Jan 9, 2020
@mark-vieira mark-vieira changed the title [CI] InternalAggregationsTests.tesSerialization reproducibly failing [CI] InternalAggregationsTests testSerialization reproducibly failing Jan 9, 2020
@cbuescher
Copy link
Member

Looks like yet again a subtle time zone glitch, only triggered when the serialization is done on a version <7.0. The time zone of the rounding inside an InternalDateHistogram is set to "Z" (Zulu time) which seems to be equivalent to UTC. For versions before 7.0 we don't directly write the zone ID but convert to a string representation which in this case gives "UTC". This is deserialized to a "UTC" time zone which is equivalent but fails our strict equality checks.
I'll mute the test for now and work on a fix.

@cbuescher cbuescher self-assigned this Jan 10, 2020
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this issue Jan 10, 2020
Fix time zone issue in Rounding serialization
79992c4 
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 mentioned this issue Jan 10, 2020
Merged
cbuescher pushed a commit that referenced this issue Jan 13, 2020
Fix time zone issue in Rounding serialization (#50845)
c31a21c 
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.

Co-authored-by: Nik Everett <[email protected]>

Closes #50827
@cbuescher
Copy link
Member

Should be closed by #50845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cbuescher cbuescher

Labels
:Search/Search Search-related issues that do not fall into other categories >test-failure Triaged test failures from CI
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mark-vieira @cbuescher @elasticmachine