Migrating from joda to java.time. Watcher plugin #35809
Conversation
This commit moves the aggregation and mapping code from joda time to java time. This includes field mappers, root object mappers, aggregations with date histograms, query builders and a lot of changes within tests.
…m property configured
| * "Asia/Kathmandu, 1 Jan 1986 - Time Zone Change (IST → NPT), at 00:00:00 clocks were turned forward 00:15 minutes | ||
| */ | ||
| // This test fails because we cannot parse negative epoch milli seconds yet... but perhaps we dont have to if we use instants in the | ||
| // rangefield method? |
There was a problem hiding this comment.
Did this change sneak in ?
There was a problem hiding this comment.
should not be here (old comment, not in master). will remove
| public void testAckStatusIsResetOnUnmetCondition() { | ||
| HashMap<String, ActionStatus> myMap = new HashMap<>(); | ||
| ActionStatus actionStatus = new ActionStatus(now()); | ||
| ActionStatus actionStatus = new ActionStatus(ZonedDateTime.now()); |
There was a problem hiding this comment.
I know these are only tests, but we can we avoid using the default TZ ? For anyone that looks at the unit tests for examples, this can lead to bugs.
There was a problem hiding this comment.
sure, good advice. I will change this and another test in this PR that is also missing this
|
Overall this change looks good a couple small comments. However a broader question, (not suggesting that we do this now), why do we pass a ZonedDateTime around everywhere if we only care about UTC timezone ? I would think that passing around LocaleDateTime and only add the UTC timezone information when/where needed (like for serialization) would be more accurate and less prone to bugs. |
|
personally I feel a bit safer knowing that all the dates used around are in UTC. Not entirely sure which places you have in mind where LocalDateTime would be better suited? Definitely worth discussing and refactoring in a next PR. |
jakelandis
left a comment
jakelandis
left a comment
There was a problem hiding this comment.
LGTM
thanks for the updates and tackling this large chunk of code.
| email.priority(Email.Priority.resolve(parser.text())); | ||
| } else if (Field.SENT_DATE.match(currentFieldName, parser.getDeprecationHandler())) { | ||
| email.sentDate(new DateTime(parser.text(), DateTimeZone.UTC)); | ||
| email.sentDate(ZonedDateTime.parse(parser.text()) |
There was a problem hiding this comment.
I'd go with a strict_date_time formatter for clarity here
15 participants
part of the migrating joda time work.
migration watcher plugin to use JDK's java-time
refers #27330