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

update _ingest.timestamp to use new ZonedDateTime #23174

Closed
wants to merge 1 commit into from

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Feb 15, 2017

Previously, Mustache would call toString on the _ingest.timestamp
field and return a date format that did not match Elasticsearch's
defaults for date-mapping parsing. The new ZonedDateTime class in Java 8
happens to do format itself in the same way ES is expecting.

Fixes #23168.

@talevy talevy added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >bug v5.4.0 v6.0.0-alpha1 labels Feb 15, 2017
@talevy talevy force-pushed the fix-23168 branch 2 times, most recently from 9e54040 to d91d01e Compare February 15, 2017 01:45
@rjernst
Copy link
Member

rjernst commented Feb 15, 2017

While I think this is the correct change long term, I do wonder what, if anything we could or should do for backcompat. Users may already have code using Date, for example in a script processor. /cc @clintongormley

@clintongormley
Copy link

Good point @rjernst. All I can think of here is:

  • In 5.4, keep the functionality as it is, but allow the new format to be enabled with a setting.
  • Add deprecation logging if the setting is not enabled.
  • In 6.0, change to the new functionality and remove the setting.

@talevy
Copy link
Contributor Author

talevy commented Feb 15, 2017

👍 will add a new_format flag or something like that, and update the docs with a discussion around the change and the use-case.

@clintongormley
Copy link

@talevy should be a setting rather than a flag i think - easier to remove when they upgrade

@talevy
Copy link
Contributor Author

talevy commented Feb 15, 2017

@clintongormley ah, I see. didn't think about going into the es settings. that is better. thanks

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Previously, Mustache would call `toString` on the `_ingest.timestamp`
field and return a date format that did not match Elasticsearch's
defaults for date-mapping parsing. The new ZonedDateTime class in Java 8
happens to do format itself in the same way ES is expecting.

Fixes elastic#23168.
@talevy
Copy link
Contributor Author

talevy commented Apr 10, 2017

#24030 was introduced to supersede this PR. I hope to merge that one into 5.x and master, then follow-up with a PR to remove the setting flag, and reduce that change to basically what we see here (just for master)

@talevy talevy closed this Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v5.4.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants