-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Core: Move to java.time in Streamable/XContent #28327
Conversation
- StreamInput/StreamOutput now use java.time to interpret date times. - XContentBuilder uses java.time for its formatters. - Cutover to using java time zones in MappedFieldType.rangeQuery (and thus RangeFieldMapper, DateFieldMapper, SimpleMappedFieldType) - QueryStringQueryBuilder and RangeQueryBuilder uses java.time time zone - A few tests were moved to java.time where simply any time was needed Relates elastic#27330
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -44,7 +46,20 @@ public DateMathParser(FormatDateTimeFormatter dateTimeFormatter) { | |||
} | |||
|
|||
public long parse(String text, LongSupplier now) { | |||
return parse(text, now, false, null); | |||
return parse(text, now, false, (DateTimeZone) null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be good (for ease of removal in the future) to flip around the joda impl below, so that it is the wrapper method, and the core parse method (which handles no time zone) is the same that handles ZoneId? This could be a follow up.
final String timeZoneId = readString(); | ||
return new DateTime(readLong(), DateTimeZone.forID(timeZoneId)); | ||
return ZonedDateTime.ofInstant(Instant.ofEpochMilli(readLong()), ZoneId.of(timeZoneId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ZoneId.of
work for a larger range of values than DateTimeZone.forID
? Or maybe we tell people not to use the ones that used to work and now don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, there are such cases, see #27330 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. So the plan is to add deprecation warnings to older versions of ES and not support them in master?
Closing, will supercede this soon with a new PR, that also contains x-pack changes. |
thus RangeFieldMapper, DateFieldMapper, SimpleMappedFieldType)
Relates #27330