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.

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

Fix timezone fallback in ingest processor #38407

Merged
merged 8 commits into from
Feb 9, 2019
Merged

Fix timezone fallback in ingest processor #38407

merged 8 commits into from
Feb 9, 2019

Conversation

spinscale
Copy link
Contributor

If no timezone was specified in the date processor, then the conversion
would lead to wrong time, as UTC was assumed by default, leading to
wrongly stored dates.

This commit does not assume a default timezone and will thus not format
the dates in a wrong way.

Unfortunately we never had any tests testing this, but this was found via the beats integration tests.

@spinscale
Fix timezone fallback in ingest processor
0b17d88 
If no timezone was specified in the date processor, then the conversion
would lead to wrong time, as UTC was assumed by default, leading to
wrongly stored dates.

This commit does not assume a default timezone and will thus not format
the dates in a wrong way.
@spinscale spinscale added >non-issue :Core/Infra/Core Core issues without another label v7.0.0 labels Feb 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

pgomulka
pgomulka approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. I think it is unlikely that Locale would be null (can't see that possibility in code)

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/DateFormat.java
.withLocale(locale)
.withZone(zoneId);
DateFormatter dateFormatter = DateFormatter.forPattern(format)
.withLocale(locale);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose locale cannot be null?
the if condition in JavaDateFormatter is reversed compared to JodaDateFormatter which might throw NPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, cannot be null, fallback is done in the date processor configuration

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019
@spinscale
Copy link
Contributor Author

@pgomulka can you take another look? I looked at the diff and figured I had reimplemented the earlier null check in the processors in the date format class. All that was needed were a few changes in the DateFormat.Java part of things... sorry for the iteration, but the code is much shorter now

pgomulka
pgomulka approved these changes Feb 7, 2019
View reviewed changes
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice - I like those extra test cases
LGTM

@spinscale spinscale merged commit c0328d5 into elastic:master Feb 9, 2019
spinscale added a commit to spinscale/elasticsearch that referenced this pull request Feb 9, 2019
@spinscale
Fix timezone fallback in ingest processor (elastic#38407)
94bda11 
If no timezone was specified in the date processor, then the conversion
would lead to wrong time, as UTC was assumed by default, leading to
incorrectly parsed dates.

This commit does not assume a default timezone and will thus not format
the dates in a wrong way.
spinscale added a commit to spinscale/elasticsearch that referenced this pull request Feb 9, 2019
@spinscale
Fix timezone fallback in ingest processor (elastic#38407)
969151e 
If no timezone was specified in the date processor, then the conversion
would lead to wrong time, as UTC was assumed by default, leading to
incorrectly parsed dates.

This commit does not assume a default timezone and will thus not format
the dates in a wrong way.
This was referenced Feb 9, 2019
Merged
Merged
spinscale added a commit that referenced this pull request Feb 9, 2019
@spinscale
Fix timezone fallback in ingest processor (#38407) (#38664)
56edc8e 
If no timezone was specified in the date processor, then the conversion
would lead to wrong time, as UTC was assumed by default, leading to
incorrectly parsed dates.

This commit does not assume a default timezone and will thus not format
the dates in a wrong way.
jasontedor added a commit to liketic/elasticsearch that referenced this pull request Feb 10, 2019
@jasontedor
Merge branch 'master' into pr/28895
e3508d1 
* master: (1159 commits)
  Fix timezone fallback in ingest processor (elastic#38407)
  Avoid polluting download stats on builds (elastic#38660)
  SQL: Prevent grouping over grouping functions (elastic#38649)
  SQL: Relax StackOverflow circuit breaker for constants (elastic#38572)
  [DOCS] Fixes broken migration links (elastic#38655)
  Drop support for the low-level REST client on JDK 7 (elastic#38540)
  [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641)
  fix dissect doc "ip" --> "clientip" (elastic#38545)
  Concurrent file chunk fetching for CCR restore (elastic#38495)
  make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473)
  SQL: Replace joda with java time (elastic#38437)
  Add fuzziness example (elastic#37194) (elastic#38648)
  Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636)
  add geotile_grid ref to asciidoc (elastic#38632)
  Enable Dockerfile from artifacts.elastic.co (elastic#38552)
  Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634)
  Account for a possible rolled over file while reading the audit log file (elastic#34909)
  Mute failure in InternalEngineTests (elastic#38622)
  Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518)
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 11, 2019
@jasontedor
Merge branch 'master' into retention-lease-ccr
baec3a0 
* master:
  Fix timezone fallback in ingest processor (elastic#38407)
  Avoid polluting download stats on builds (elastic#38660)
  SQL: Prevent grouping over grouping functions (elastic#38649)
  SQL: Relax StackOverflow circuit breaker for constants (elastic#38572)
  [DOCS] Fixes broken migration links (elastic#38655)
  Drop support for the low-level REST client on JDK 7 (elastic#38540)
  [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641)
  fix dissect doc "ip" --> "clientip" (elastic#38545)
  Concurrent file chunk fetching for CCR restore (elastic#38495)
  make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473)
  SQL: Replace joda with java time (elastic#38437)
  Add fuzziness example (elastic#37194) (elastic#38648)
@pgomulka pgomulka mentioned this pull request Oct 19, 2020
Merged
pgomulka added a commit that referenced this pull request Oct 26, 2021
@pgomulka
Fix ingest timezone parsing (#63876)
226f116 
this commit aligns timezone parsing logic with DateFormat.Iso8601. Timezone can be provided on a pattern and as ingest parameter. When no timezone is provided on a pattern it is defaulted to UTC or an ingest parameter (if provided).

When timezone is not present in a pattern, but an ingest timezone parameter is provided - a date should be parsed as if it was in that timezone.
Example: pattern "uuuu-MM-dd'T'HH:mm", text "2020-01-01T01:00", timezone: -01:00 should return 2020-01-01T01:00:00-01:00
If the pattern has a timezone and a timezone parameter is provided - a date should parsed with a timezone from input, and then "recalculated" to an ingest parameter
Example "uuuu-MM-dd'T'HH:mm XXX", text "2020-01-01T01:00 -02:00", timezone -01:00 should return 2020-01-01T02:00:00-01:00
relates #38407
relates #51215
closes #63458
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Oct 26, 2021
@pgomulka
Fix ingest timezone parsing (elastic#63876)
52e4e73 
this commit aligns timezone parsing logic with DateFormat.Iso8601. Timezone can be provided on a pattern and as ingest parameter. When no timezone is provided on a pattern it is defaulted to UTC or an ingest parameter (if provided).

When timezone is not present in a pattern, but an ingest timezone parameter is provided - a date should be parsed as if it was in that timezone.
Example: pattern "uuuu-MM-dd'T'HH:mm", text "2020-01-01T01:00", timezone: -01:00 should return 2020-01-01T01:00:00-01:00
If the pattern has a timezone and a timezone parameter is provided - a date should parsed with a timezone from input, and then "recalculated" to an ingest parameter
Example "uuuu-MM-dd'T'HH:mm XXX", text "2020-01-01T01:00 -02:00", timezone -01:00 should return 2020-01-01T02:00:00-01:00
relates elastic#38407
relates elastic#51215
closes elastic#63458
@pgomulka pgomulka mentioned this pull request Oct 26, 2021
Merged
pgomulka added a commit that referenced this pull request Oct 27, 2021
@pgomulka
Fix ingest timezone parsing (#63876) backport(#79838)
b58f5fe 
this commit aligns timezone parsing logic with DateFormat.Iso8601. Timezone can be provided on a pattern and as ingest parameter. When no timezone is provided on a pattern it is defaulted to UTC or an ingest parameter (if provided).

When timezone is not present in a pattern, but an ingest timezone parameter is provided - a date should be parsed as if it was in that timezone.
Example: pattern "uuuu-MM-dd'T'HH:mm", text "2020-01-01T01:00", timezone: -01:00 should return 2020-01-01T01:00:00-01:00
If the pattern has a timezone and a timezone parameter is provided - a date should parsed with a timezone from input, and then "recalculated" to an ingest parameter
Example "uuuu-MM-dd'T'HH:mm XXX", text "2020-01-01T01:00 -02:00", timezone -01:00 should return 2020-01-01T02:00:00-01:00
relates #38407
relates #51215
closes #63458

backports #63876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgomulka pgomulka pgomulka approved these changes

Assignees
No one assigned
Labels
blocker :Core/Infra/Core Core issues without another label >non-issue v7.0.0-beta1 v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@spinscale @elasticmachine @pgomulka @jakelandis @danielmitterdorfer @jasontedor @jimczi @tomcallahan