Core: Migrating from joda to java.time. Monitoring plugin by pgomulka · Pull Request #36297 · elastic/elasticsearch

pgomulka · 2018-12-06T09:32:05Z

monitoring plugin migration from joda to java.time

elasticmachine · 2018-12-06T09:32:07Z

Pinging @elastic/es-core-infra

pgomulka · 2019-01-28T14:21:41Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/monitoring/exporter/MonitoringDoc.java

     * @return a string representing the timestamp
     */
    public static String toUTC(final long timestamp) {
-        return new DateTime(timestamp, DateTimeZone.UTC).toString();


joda toString:
Output the date time in ISO8601 format (yyyy-MM-ddTHH:mm:ss.SSSZZ).
when java-time
{@code 2007-12-03T10:15:30+01:00[Europe/Paris]}.
but it also says: The output is compatible with ISO-8601 if the offset and ID are the same.
The quick test for sample long shows that the format is the same
Instant.ofEpochMilli(123).atZone(ZoneOffset.UTC).toString()
new DateTime(123, DateTimeZone.UTC).toString();
1970-01-01T00:00:00.123Z

can we make this more explicit and not rely on the toString() method of classes we dont have control over (even though I assume this does not get changed) Using a strict_date_time formatter might just work as expected?

it should work fine - the pattern is the same

...src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java

...src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java

danielmitterdorfer

Looks fine overall but I think I spotted one unrelate change.

...re/src/main/java/org/elasticsearch/xpack/core/security/action/user/AuthenticateResponse.java

pgomulka · 2019-01-28T17:30:23Z

@elasticmachine run elasticsearch-ci/1

pgomulka · 2019-01-28T17:30:34Z

@elasticmachine run elasticsearch-ci/default-distro

danielmitterdorfer

LGTM.

pgomulka · 2019-01-29T07:23:01Z

ok to test

spinscale

left a few comments, mostly about week year based format that should be replaced

spinscale · 2019-01-29T08:33:54Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/monitoring/exporter/MonitoringDoc.java

     * @return a string representing the timestamp
     */
    public static String toUTC(final long timestamp) {
-        return new DateTime(timestamp, DateTimeZone.UTC).toString();


can we make this more explicit and not rely on the toString() method of classes we dont have control over (even though I assume this does not get changed) Using a strict_date_time formatter might just work as expected?

spinscale · 2019-01-29T08:36:16Z

...ugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/cleaner/CleanerService.java

    protected void doStart() {
        logger.debug("starting cleaning service");
-        threadPool.schedule(executionScheduler.nextExecutionDelay(new DateTime(ISOChronology.getInstance())), executorName(), runnable);
+        threadPool.schedule(executionScheduler.nextExecutionDelay(ZonedDateTime.now(Clock.systemDefaultZone())),


can we use Clock.systemUTC() here? I see that we did not do this properly before though... see the DateTime() ctor

spinscale · 2019-01-29T08:36:51Z

...ugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/cleaner/CleanerService.java

        @Override
        protected void onAfterInLifecycle() {
-            DateTime start = new DateTime(ISOChronology.getInstance());
+            ZonedDateTime start = ZonedDateTime.now(Clock.systemDefaultZone());


Clock.systemUTC()?

spinscale · 2019-01-29T08:44:24Z

...src/test/java/org/elasticsearch/xpack/monitoring/cleaner/AbstractIndicesCleanerTestCase.java

-        final DateTimeFormatter formatter = DateTimeFormat.forPattern("YYYY.MM.dd").withZoneUTC();
-        final String index = ".watcher-history-" + version + "-" + formatter.print(creationDate.getMillis());
+    protected void createWatcherHistoryIndex(final ZonedDateTime creationDate, final String version) {
+        DateFormatter formatter = DateFormatter.forPattern("YYYY.MM.dd").withZone(ZoneOffset.UTC);


YYYY in java time means week based year, where is in joda time it means year of era - this is lower case yyyy in java time

see https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html
see https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html

spinscale · 2019-01-29T08:45:01Z

...src/test/java/org/elasticsearch/xpack/monitoring/cleaner/AbstractIndicesCleanerTestCase.java

-        final DateTimeFormatter formatter = DateTimeFormat.forPattern("YYYY.MM.dd").withZoneUTC();
-        final String index = ".watcher-history-" + version + "-" + formatter.print(creationDate.getMillis());
+    protected void createWatcherHistoryIndex(final ZonedDateTime creationDate, final String version) {
+        DateFormatter formatter = DateFormatter.forPattern("YYYY.MM.dd").withZone(ZoneOffset.UTC);


does it make sense to make the formatter static?

spinscale · 2019-01-29T08:46:30Z

...src/test/java/org/elasticsearch/xpack/monitoring/cleaner/AbstractIndicesCleanerTestCase.java

-        final DateTimeFormatter formatter = DateTimeFormat.forPattern("YYYY.MM.dd").withZoneUTC();
-        final String index = ".watcher-history-" + version + "-" + formatter.print(creationDate.getMillis());
+    protected void createWatcherHistoryIndex(final ZonedDateTime creationDate, final String version) {
+        DateFormatter formatter = DateFormatter.forPattern("YYYY.MM.dd").withZone(ZoneOffset.UTC);


does it make sense to add a test for this corner case in order to catch it?

hmm this is a test class, what do you suggest to catch?

indeed, nevermind.

spinscale · 2019-01-29T08:47:49Z

.../src/test/java/org/elasticsearch/xpack/monitoring/exporter/MonitoringTemplateUtilsTests.java

+            0, ZoneOffset.UTC).toInstant().toEpochMilli();

-        DateTimeFormatter formatter = DateTimeFormat.forPattern("YYYY.MM.dd").withZoneUTC();
+        DateFormatter formatter = DateFormatter.forPattern("YYYY.MM.dd").withZone(ZoneOffset.UTC);


week based formatter

spinscale · 2019-01-29T08:48:09Z

.../src/test/java/org/elasticsearch/xpack/monitoring/exporter/MonitoringTemplateUtilsTests.java

                equalTo(".monitoring-beats-" + TEMPLATE_VERSION + "-2017.08.03"));

-        formatter = DateTimeFormat.forPattern("YYYY-dd-MM-HH.mm.ss").withZoneUTC();
+        formatter = DateFormatter.forPattern("YYYY-dd-MM-HH.mm.ss").withZone(ZoneOffset.UTC);


week based formatter

spinscale · 2019-01-29T08:48:16Z

...onitoring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpExporterIT.java

        MockRequest recordedRequest = assertBulk(webServer);

-        String indexName = indexName(DateTimeFormat.forPattern("YYYY.MM.dd").withZoneUTC(), doc.getSystem(), doc.getTimestamp());
+        DateFormatter formatter = DateFormatter.forPattern("YYYY.MM.dd").withZone(ZoneOffset.UTC);


week based formatter

spinscale · 2019-01-29T08:48:36Z

...in/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/integration/MonitoringIT.java

-        assertThat(index, equalTo(MonitoringTemplateUtils.indexName(DateTimeFormat.forPattern("YYYY.MM.dd").withZoneUTC(),
-                                                                    expectedSystem,
-                                                                    ISODateTimeFormat.dateTime().parseMillis(timestamp))));
+        DateFormatter formatter = DateFormatter.forPattern("YYYY.MM.dd");


week based formatter

spinscale · 2019-02-01T13:17:12Z

I think the most important date was not changed in the PR and this can suffer from the weekyear issue: the formatter string of the exporter that sends the data to the local cluster or a remote one.

I put this test in org.elasticsearch.xpack.monitoring.exporter.ExportersTests and it fails

    public void testDateFormatter() {
        Exporter.Config config = mock(Exporter.Config.class);
        when(config.name()).thenReturn("anything");
        when(config.settings()).thenReturn(Settings.EMPTY);
        DateFormatter formatter = Exporter.dateTimeFormatter(config);
        Instant instant = Instant.ofEpochSecond(randomLongBetween(0, 86400 * 365 * 130L));
        ZonedDateTime zonedDateTime = instant.atZone(ZoneOffset.UTC);
        int year = zonedDateTime.getYear();
        int month = zonedDateTime.getMonthValue();
        int day = zonedDateTime.getDayOfMonth();
        String expecdateDate = String.format(Locale.ROOT, "%02d.%02d.%02d", year, month, day);
        String formattedDate = formatter.format(instant);
        assertThat("input date was " + instant, expecdateDate, is(formattedDate));
    }

I think we need to fix Exporter.INDEX_FORMAT

spinscale · 2019-02-04T09:11:41Z

...ugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/cleaner/CleanerService.java

                next = next.plusDays(1);
            }
-            return TimeValue.timeValueMillis(next.getMillis() - now.getMillis());
+            return TimeValue.timeValueMillis(next.toInstant().toEpochMilli() - now.toInstant().toEpochMilli());


minor nit: you could simplify this to Duration.between(now, next).toMillis() (pay attention that you have to switch the order in that case)

spinscale · 2019-02-04T09:13:56Z

...ck/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java

        String format = setting.exists(config.settings()) ? setting.get(config.settings()) : INDEX_FORMAT;
        try {
-            return DateTimeFormat.forPattern(format).withZoneUTC();
+            return DateFormatter.forPattern(format).withZone(ZoneOffset.UTC);


this also requires a change in 6.7, otherwise users using outdated formats will not be warned if they do so, as this uses the joda time API directly

spinscale · 2019-02-04T09:47:03Z

FYI: I created #38309 for the required 6.7 change

pgomulka added WIP :Core/Infra/Core Core issues without another label labels Dec 6, 2018

pgomulka self-assigned this Dec 6, 2018

pgomulka force-pushed the feature/monitoring branch 5 times, most recently from 09de1e0 to 13846f5 Compare December 13, 2018 16:14

pgomulka force-pushed the feature/monitoring branch 2 times, most recently from ee5a5d8 to 96109e1 Compare January 25, 2019 11:06

spinscale mentioned this pull request Jan 25, 2019

Core: Migrating from joda time to java.time #27330

Closed

18 tasks

pgomulka force-pushed the feature/monitoring branch from 96109e1 to b1f2cc6 Compare January 25, 2019 14:06

Joda to java-time migration in Monitoring

93541a7

pgomulka force-pushed the feature/monitoring branch from b1f2cc6 to 93541a7 Compare January 25, 2019 15:18

pgomulka commented Jan 28, 2019

View reviewed changes

...src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java Outdated Show resolved Hide resolved

pgomulka added 4 commits January 28, 2019 15:24

Merge branch 'master' into feature/monitoring

83c024c

remove todo

438ce82

reverting empty lines

4cdcbf8

revert back to prev rev

40b47a6

pgomulka commented Jan 28, 2019

View reviewed changes

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java Show resolved Hide resolved

pgomulka changed the title ~~Work In Progress: Core: Migrating from joda to java.time. Monitoring plugin~~ Core: Migrating from joda to java.time. Monitoring plugin Jan 28, 2019

pgomulka requested review from danielmitterdorfer and spinscale January 28, 2019 14:29

pgomulka removed the WIP label Jan 28, 2019

pgomulka commented Jan 28, 2019

View reviewed changes

...src/test/java/org/elasticsearch/xpack/monitoring/exporter/local/LocalExporterIntegTests.java Show resolved Hide resolved

Update RestSqlTranslateAction.java

19aff93

danielmitterdorfer reviewed Jan 28, 2019

View reviewed changes

...re/src/main/java/org/elasticsearch/xpack/core/security/action/user/AuthenticateResponse.java Outdated Show resolved Hide resolved

...re/src/main/java/org/elasticsearch/xpack/core/security/action/user/AuthenticateResponse.java Outdated Show resolved Hide resolved

remove uneeded changes

7130b85

danielmitterdorfer approved these changes Jan 29, 2019

View reviewed changes

Merge branch 'master' into feature/monitoring

0850175

spinscale requested changes Jan 29, 2019

View reviewed changes

pgomulka added 6 commits January 31, 2019 11:03

code review follow up

41da102

checkstyle fix

dac6924

Merge branch 'master' into feature/monitoring

2e7f180

Merge branch 'master' into feature/monitoring

1c2d553

static field in test cleanup

88f56bc

Merge branch 'master' into feature/monitoring

9272269

fix the exporter index pattern

33cd6dd

spinscale approved these changes Feb 4, 2019

View reviewed changes

small cleanup after review

1e1198f

Merge branch 'master' into feature/monitoring

ab9032a

pgomulka merged commit 85b4bfe into elastic:master Feb 4, 2019

danielmitterdorfer added >non-issue v7.0.0 labels Feb 7, 2019

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

ChrisHegarty unassigned pgomulka Oct 29, 2024

Conversation

pgomulka commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pgomulka commented Jan 28, 2019

Uh oh!

pgomulka commented Jan 28, 2019

Uh oh!

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Jan 29, 2019

Uh oh!

spinscale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spinscale commented Feb 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spinscale commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pgomulka commented Dec 6, 2018 •

edited

Loading