Date: Add DateFormatters class that uses java.time #31856

spinscale · 2018-07-06T09:40:12Z

A newly added class called DateFormatters now contains java.time based
builders for dates, which also intends to be fully backwards compatible,
when the name based date formatters are picked.

A duelling test class has been added that ensures the same dates when
parsing java or joda time formatted dates for the name based dates.

A first user of this class is the ingest module, which now uses
java.time under the hood instead of joda time.

Note, that java.time and joda time are not fully backwards compatible,
which also means that old formats will currently not work with this
setup.

Note: This is does not include the joda to java-time conversions in the ingest processors done in #30612, just adding the new class.

A newly added class called DateFormatters now contains java.time based builders for dates, which also intends to be fully backwards compatible, when the name based date formatters are picked. A duelling test class has been added that ensures the same dates when parsing java or joda time formatted dates for the name based dates. A first user of this class is the ingest module, which now uses java.time under the hood instead of joda time. Note, that java.time and joda time are not fully backwards compatible, which also means that old formats will currently not work with this setup.

elasticmachine · 2018-07-06T09:40:14Z

Pinging @elastic/es-core-infra

spinscale · 2018-07-06T11:35:05Z

@elasticmac please retest this

spinscale · 2018-07-06T14:20:10Z

@elasticmachine please retest this

rjernst

Thanks Alex, the tests are great. I do think we should discuss whether we should maintain these named formats long term, but this is great for backcompat in the meantime. I have some general requests.

rjernst · 2018-07-06T23:53:58Z

server/src/main/java/org/elasticsearch/common/time/DateFormatter.java

+    }
+
+    public TemporalAccessor parse(String input) {
+        if (parsers.length > 0) {


Can you check this in the ctor? If no parsers are passed, then set parsers to a single sized array of printer.

rjernst · 2018-07-07T00:09:18Z

server/src/main/java/org/elasticsearch/common/time/DateFormatter.java

+/**
+ * wrapper class around java.time.DateTimeFormatter that supports multiple formats for easier parsing
+ */
+public class DateFormatter {


Can this be named more transparently, like CompoundDateTimeFormatter?

rjernst · 2018-07-07T00:09:41Z

server/src/main/java/org/elasticsearch/common/time/DateFormatter.java

+import java.util.stream.Stream;
+
+/**
+ * wrapper class around java.time.DateTimeFormatter that supports multiple formats for easier parsing


I think this should be more clear the multiple formats has to do with parsing, not printing.

updated the comment

rjernst · 2018-07-07T00:11:17Z

server/src/main/java/org/elasticsearch/common/time/DateFormatter.java

+        return printer.parse(input);
+    }
+
+    public DateFormatter withZone(ZoneId zoneId) {


I think this can be implemented in the same way, but by calling the constructor with the resulting formatters? Then the formatter members can be final.

formatters are now final

rjernst · 2018-07-07T00:17:00Z

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

+        .append(TIME_NO_MILLIS_FORMATTER_3)
+        .toFormatter(Locale.ROOT);
+
+    private static final DateFormatter T_TIME_NO_MILLIS = new DateFormatter(T_TIME_NO_MILLIS_FORMATTER_1, T_TIME_NO_MILLIS_FORMATTER_2,


Is T_TIME_NO_MILLIS_FORMATTER_1 really only meant for printing, not parsing?

made it more clear that the first argument is a printer as well as a parser, where as the others are only parsers

rjernst · 2018-07-07T00:21:08Z

server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

+                    }
+                }
+
+                return DateFormatter.merge(formatters);


I think this merge method could go away altogether if the printer and parsers members are made final and package private?

indeed. removed

rjernst

LGTM. I just have one final question.

rjernst · 2018-07-09T15:32:39Z

server/src/main/java/org/elasticsearch/common/time/CompoundDateTimeFormatter.java

+    final DateTimeFormatter[] parsers;
+
+    CompoundDateTimeFormatter(DateTimeFormatter parserAndPrinter, DateTimeFormatter ... parsers) {
+        if (parserAndPrinter == null) {


This name seems deceptive because if you pass any parsers, then this is not used as a parser, right? If it should also always be a parser, then I think it would be better to just have a formatters member, and use formatters[0] for printing.

indeed, way better. I'll push once CI passes... thx for reviewing!

…he printer, no need for more complex logic in the date formatter

rjernst · 2018-07-11T01:22:11Z

@spinscale I think this can be backported to 6.x right? It will help there with providing a bridge between java and joda.

* master: [TEST] Mute SlackMessageTests.testTemplateRender Docs: Explain closing the high level client [ML] Re-enable memory limit integration tests (#31328) [test] disable packaging tests for suse boxes Add nio transport to security plugin (#31942) XContentTests : Insert random fields at random positions (#30867) Force execution of fetch tasks (#31974) Fix unreachable error condition in AmazonS3Fixture (#32005) Tests: Fix SearchFieldsIT.testDocValueFields (#31995) Add Expected Reciprocal Rank metric (#31891) [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973) SQL: Add support for single parameter text manipulating functions (#31874) [ML] Ensure immutability of MlMetadata (#31957) Tests: Mute SearchFieldsIT.testDocValueFields() muted tests due to #31940 Work around reported problem in eclipse (#31960) Move build integration tests out of :buildSrc project (#31961) Tests: Remove use of joda time in some tests (#31922) [Test] Reactive 3rd party tests on CI (#31919) SQL: Support for escape sequences (#31884) SQL: HAVING clause should accept only aggregates (#31872) Docs: fix typo in datehistogram (#31972) Switch url repository rest tests to new style requests (#31944) Switch reindex tests to new style requests (#31941) Docs: Added note about cloud service to installation and getting started [DOCS] Removes alternative docker pull example (#31934) Add Snapshots Status API to High Level Rest Client (#31515) ingest: date_index_name processor template resolution (#31841) Test: fix null failure in watcher test (#31968) Switch test framework to new style requests (#31939) Switch low level rest tests to new style Requests (#31938) Switch high level rest tests to new style requests (#31937) [ML] Mute test failing due to Java 11 date time format parsing bug (#31899) [TEST] Mute SlackMessageTests.testTemplateRender Fix assertIngestDocument wrongfully passing (#31913) Remove unused reference to filePermissionsCache (#31923) rolling upgrade should use a replica to prevent relocations while running a scroll HLREST: Bundle the x-pack protocol project (#31904) Increase logging level for testStressMaybeFlush Added lenient flag for synonym token filter (#31484) [X-Pack] Beats centralized management: security role + licensing (#30520) HLRest: Move xPackInfo() to xPack().info() (#31905) Docs: add security delete role to api call table (#31907) [test] port archive distribution packaging tests (#31314) Watcher: Slack message empty text (#31596) [ML] Mute failing DetectionRulesIT.testCondition() test Fix broken NaN check in MovingFunctions#stdDev() (#31888) Date: Add DateFormatters class that uses java.time (#31856) [ML] Switch native QA tests to a 3 node cluster (#31757) Change trappy float comparison (#31889) Fix building AD URL from domain name (#31849) Add opaque_id to audit logging (#31878) re-enable backcompat tests add support for is_write_index in put-alias body parsing (#31674) Improve release notes script (#31833) [DOCS] Fix broken link in painless example Handle missing values in painless (#30975) Remove the ability to index or query context suggestions without context (#31007) Ingest: Enable Templated Fieldnames in Rename (#31690) [Docs] Fix typo in the Rollup API Quick Reference (#31855) Ingest: Add ignore_missing option to RemoveProc (#31693) Add template config for Beat state to X-Pack Monitoring (#31809) Watcher: Add ssl.trust email account setting (#31684) Remove link to oss-MSI (#31844) Painless: Restructure Definition/Whitelist (#31879) HLREST: Add x-pack-info API (#31870)

A newly added class called DateFormatters now contains java.time based builders for dates, which also intends to be fully backwards compatible, when the name based date formatters are picked. Also a new class named CompoundDateTimeFormatter for being able to parse multiple different formats has been added. A duelling test class has been added that ensures the same dates when parsing java or joda time formatted dates for the name based dates. Note, that java.time and joda time are not fully backwards compatible, which also means that old formats will currently not work with this setup.

* Date: Add DateFormatters class that uses java.time (#31856) A newly added class called DateFormatters now contains java.time based builders for dates, which also intends to be fully backwards compatible, when the name based date formatters are picked. Also a new class named CompoundDateTimeFormatter for being able to parse multiple different formats has been added. A duelling test class has been added that ensures the same dates when parsing java or joda time formatted dates for the name based dates. Note, that java.time and joda time are not fully backwards compatible, which also means that old formats will currently not work with this setup. * Tests: Remove use of joda time in some tests (#31922) This also extends the dateformatters test to ensure that the printers are acting the same in java time and joda time. * Tests: Fix SearchFieldsIT.testDocValueFields This test produced different implementations of joda time classes, depending on if the data was serialized or not (DateTime vs MutableDateTime). This now uses a common base class to extract the milliseconds from the data. Closes #31992

spinscale added review :Core/Infra/Core Core issues without another label v7.0.0 labels Jul 6, 2018

spinscale requested a review from rjernst July 6, 2018 09:40

Merge branch 'master' into 1807-java-time-dateformatters

715137d

rjernst requested changes Jul 7, 2018

View reviewed changes

spinscale added 4 commits July 9, 2018 10:28

review comment: rename DateFormatter to CompoundDateTimeFormatter

5007a93

review comment: make printer/parsers final

cd1e2df

review comment: Remove DateFormatter.merge()

fc24da8

fix checkstyle

e37d99a

rjernst approved these changes Jul 9, 2018

View reviewed changes

review comment: only have a list of parsers, where the first one is t…

d37c2ca

…he printer, no need for more complex logic in the date formatter

spinscale merged commit 1c32497 into elastic:master Jul 10, 2018

spinscale mentioned this pull request Jul 12, 2018

Date: Add DateFormatters class that uses java.time #30612

Closed

rjernst added the v6.4.0 label Jul 12, 2018

spinscale added the >non-issue label Aug 20, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Date: Add DateFormatters class that uses java.time #31856

Date: Add DateFormatters class that uses java.time #31856

spinscale commented Jul 6, 2018

elasticmachine commented Jul 6, 2018

spinscale commented Jul 6, 2018

spinscale commented Jul 6, 2018

rjernst left a comment

rjernst Jul 6, 2018

spinscale Jul 9, 2018

rjernst Jul 7, 2018

spinscale Jul 9, 2018

rjernst Jul 7, 2018

spinscale Jul 9, 2018

rjernst Jul 7, 2018

spinscale Jul 9, 2018

rjernst Jul 7, 2018

spinscale Jul 9, 2018

rjernst Jul 7, 2018

spinscale Jul 9, 2018

rjernst left a comment

rjernst Jul 9, 2018

spinscale Jul 9, 2018

rjernst commented Jul 11, 2018

Date: Add DateFormatters class that uses java.time #31856

Date: Add DateFormatters class that uses java.time #31856

Conversation

spinscale commented Jul 6, 2018

elasticmachine commented Jul 6, 2018

spinscale commented Jul 6, 2018

spinscale commented Jul 6, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Jul 11, 2018