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

Switch default time format for ingest from Joda to Java for v7 #37934

Merged

Conversation

droberts195
Copy link
Contributor

@droberts195 droberts195 commented Jan 28, 2019

Date formats with and without the "8" prefix are now all treated
as Java time formats, so that ingest does the same as mappings
in this respect.

Relates #27330

Date formats with and without the "8" prefix are now all treated
as Java time formats, so that ingest does the same as mappings
in this respect.
@droberts195 droberts195 added :Core/Infra/Core Core issues without another label v7.0.0 labels Jan 28, 2019
@droberts195 droberts195 requested a review from spinscale January 28, 2019 16:43
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@droberts195
Copy link
Contributor Author

I opened issue #37935 to discuss side effects of the fact that this change also changes the formats used for date index naming.

Copy link
Contributor

@spinscale spinscale left a comment

Choose a reason for hiding this comment

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

I left one comment, as we need to do some more work here, but I can just do that as well.

ZonedDateTime defaultZonedDateTime = Instant.EPOCH.atZone(ZoneOffset.UTC).withYear(year);
TemporalAccessor accessor = formatter.parse(text);
long millis = DateFormatters.toZonedDateTime(accessor, defaultZonedDateTime).toInstant().toEpochMilli();
return new DateTime(millis, timezone);
Copy link
Contributor

Choose a reason for hiding this comment

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

we are still returned a joda time class here, that we need to get rid of. I am happy to fix this in a follow up PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @spinscale. I'd prefer to leave that for a followup PR as I'm sure removing Joda classes will spread out far and wide through the codebase.

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

@droberts195
Copy link
Contributor Author

I'll hold off from merging this until somebody has commented on #37935.

@droberts195 droberts195 merged commit 2f7776c into elastic:master Jan 30, 2019
@droberts195 droberts195 deleted the java_time_default_for_ingest_in_7 branch January 30, 2019 16:26
jasontedor added a commit to dnhatn/elasticsearch that referenced this pull request Jan 30, 2019
* master:
  Expose retention leases in shard stats (elastic#37991)
  Make primary terms fields private in index shard (elastic#38036)
  ML: Add reason field in JobTaskState (elastic#38029)
  Log flush_stats and commit_stats in testMaybeFlush
  HLRC: Fix strict setting exception handling (elastic#37247)
  Test: Enable strict deprecation on all tests (elastic#36558)
  Removes typed calls from YAML REST tests (elastic#37611)
  Switch default time format for ingest from Joda to Java for v7 (elastic#37934)
  Remove deprecated Plugin#onModule extension points (elastic#37866)
  Geo: Fix Empty Geometry Collection Handling (elastic#37978)
@jimczi jimczi added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP and removed :Core/Infra/Core Core issues without another label labels Feb 11, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants