Core: Fix epoch millis java time formatter #33302
Conversation
The existing implemention could not deal with negative numbers as well as +- 999 milliseconds around the epoch. This commit uses Instant.ofEpochMilli() and tries to parse the input to a number instead of using a date formatter.
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
@spinscale looks good, but I played around with the output of the formatter a bit for negative numbers and ran into some unexpected results. I left an example, maybe you can comment?
|
|
||
| private static final class EpochDateTimeFormatter extends CompoundDateTimeFormatter { | ||
|
|
||
| EpochDateTimeFormatter() { |
There was a problem hiding this comment.
nit: could even be private I think
There was a problem hiding this comment.
the class was already private... but fixed it for both ctors two as well
| super(EPOCH_MILLIS_FORMATTER); | ||
| } | ||
|
|
||
| EpochDateTimeFormatter(ZoneId zoneId) { |
There was a problem hiding this comment.
nit: could even be private I think
| @@ -879,11 +881,42 @@ | |||
|
|
|||
| /* | |||
| * Returns a formatter for parsing the milliseconds since the epoch | |||
| * This one needs a custom implementation, because the standard date formatter can not parse negative values | |||
| * or anything +- 999 milliseconds around the eopch | |||
| */ | ||
| private static final CompoundDateTimeFormatter EPOCH_MILLIS = new CompoundDateTimeFormatter(new DateTimeFormatterBuilder() | ||
| private static final DateTimeFormatter EPOCH_MILLIS_FORMATTER = new DateTimeFormatterBuilder() | ||
| .appendValue(ChronoField.INSTANT_SECONDS, 1, 19, SignStyle.NEVER) |
There was a problem hiding this comment.
The SignStyle made me curious so I tried outputting a parsed negative instance using this formatter like:
CompoundDateTimeFormatter formatter = DateFormatters.forPattern("epoch_millis");
TemporalAccessor parsed = formatter.parse("-10");
System.out.println(parsed);
System.out.println(formatter.format(parsed));
This gives me
1969-12-31T23:59:59.990Z
-1990
So the parsed instance is what I would expect, but the formatted value looks odd. I think this means the format method might be overwritten somehow, or maybe this can be done with the DateTimeFormatterBuilder()?
In any case, I think a test that parses negative values and outputs them using this formatter and checks for equality might be useful in any case.
There was a problem hiding this comment.
Good call. It seems that the date time formatter cannot deal with negative dates... overwriting the format() is something we can do in the custom implementation though., which I just did
The existing implemention could not deal with negative numbers as well as +- 999 milliseconds around the epoch. This commit uses Instant.ofEpochMilli() and parses the input to a number instead of using a date formatter.
* master: (197 commits) Prevent NPE parsing the stop datafeed request. (elastic#33347) HLRC: Add ML get overall buckets API (elastic#33297) Core: Fix epoch millis java time formatter (elastic#33302) [Docs] Improve tuning for speed advice (elastic#33315) [Rollup] Fix Caps Comparator to handle calendar/fixed time (elastic#33336) [CI] Mute IndexShardTests#testIndexCheckOnStartup fails elastic#33345 [CI] Mute LuceneChangesSnapshotTests#testUpdateAndReadChangesConcurrently Security for _field_names field should not override field statistics (elastic#33261) Add early termination support to BucketCollector (elastic#33279) Fix extractjar task ci (elastic#33272) Mute testFollowIndexAndCloseNode Logging: Drop Settings from some logging ctors (elastic#33332) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output ...
The existing implemention could not deal with negative numbers as well
as +- 999 milliseconds around the epoch.
This commit uses Instant.ofEpochMilli() and tries to parse the input as
a number directly instead of using a date formatter.