Core: Migrating from joda to java.time. ML package #35949

pgomulka · 2018-11-27T14:59:53Z

merging against java-time branch to be able to test with elasticsearch already using java.time classes.

part of the migrating joda time work. The goal is to find usages of joda
time and refactor to use java.time.

closes: #35490
previous branch against master (on top of java-time)
relates: #35493

elasticmachine · 2018-11-27T15:13:21Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-11-27T15:13:22Z

Pinging @elastic/ml-core

pgomulka · 2018-11-27T15:28:55Z

...lugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/ExpiredForecastsRemover.java

@@ -66,7 +68,7 @@
    public ExpiredForecastsRemover(Client client, ThreadPool threadPool) {
        this.client = Objects.requireNonNull(client);
        this.threadPool = Objects.requireNonNull(threadPool);
-        this.cutoffEpochMs = DateTime.now(ISOChronology.getInstance()).getMillis();
+        this.cutoffEpochMs = Instant.now().toEpochMilli();


as per discussion in original review, chronology does not matter when calculating milliseconds from epoch

But it is a default timezone..

pgomulka · 2018-11-27T15:29:52Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlDailyMaintenanceService.java

+            .atStartOfDay(now.getZone())
+            .plusMinutes(30)
+            .plusMinutes(minutesOffset);
+        return TimeValue.timeValueMillis(next.toInstant().toEpochMilli() - now.toInstant().toEpochMilli());


we did discuss to use just Instant.now(), but here I think it is better to use ZonedDateTime since it allows to use convenient DSL for calculating next variable

part of the migrating joda time work. The goal is to find usages of joda time and refactor to use java.time. closes: elastic#35490

spinscale

one major ask regarding specifying timezones when using ZonedDateTime, otherwise I think we are pretty much there

spinscale · 2018-11-28T09:06:35Z

...le-node-tests/src/test/java/org/elasticsearch/xpack/ml/transforms/PainlessDomainSplitIT.java

@@ -266,7 +267,7 @@ public void testHRDSplit() throws Exception {
                "\"time\": { \"type\": \"date\" } } }");

        // Index some data
-        DateTime baseTime = new DateTime().minusYears(1);
+        ZonedDateTime baseTime = ZonedDateTime.now().minusYears(1);


ZonedDateTime.now(ZoneOffset.UTC) should be used, as IIRC the system timezone will be used otherwise

you are right, the system default timezone will be used. But wouldn't this be change of behaviour if we use the UTC here? it was using new DateTime() previously - meaning system default time zone was used as well

For this particular test the timezone shouldn't matter provided the text representation of the timestamps includes the timezone and the test doesn't cross the start/end of daylight saving time. (I think the bit about not spanning daylight saving is a bug in the current version of the test.)

So I don't think it would be a problem to always do the test in UTC. This is not primarily testing date/time functionality so there's no need to get coverage of different timezones. It's testing completely different functionality but just needs to generate incrementing timestamps because anomaly detection deals with time series data.

thanks a ton for the clarification Dave, much appreciated!

I'd like to add all methods not specifying a timezone to the forbidden apis over time, thus my request.

spinscale · 2018-11-28T09:07:07Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlDailyMaintenanceService.java

-        DateTime next = now.plusDays(1).withTimeAtStartOfDay().plusMinutes(30).plusMinutes(minutesOffset);
-        return TimeValue.timeValueMillis(next.getMillis() - now.getMillis());
+
+        ZonedDateTime now = ZonedDateTime.now();


here again originally no time zone was specified.
DateTime.now(ISOChronology.getInstance()); is an equivalent of DateTime.now()
ISOChronology is a default and system default time zone will be defaulted too.

@droberts195 would you be able to comment on these? Do you think it is safe if we refactor this to use UTC timezones, given it wasn't previously specified?

This one really is intended to be in the system timezone. The reason is that the daily maintenance is supposed to happen during the night on the assumption that the night is when other load is lowest. (Of course, it's possible that assumption is wrong for some clusters.)

If the intention is to ban now() with no arguments then I guess this would achieve the same but specifying an argument:

ZonedDateTime now = ZonedDateTime.now(ZoneId.systemDefault());

spinscale · 2018-11-28T09:07:28Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlDailyMaintenanceService.java

+            .atStartOfDay(now.getZone())
+            .plusMinutes(30)
+            .plusMinutes(minutesOffset);
+        return TimeValue.timeValueMillis(next.toInstant().toEpochMilli() - now.toInstant().toEpochMilli());


spinscale · 2018-11-28T09:09:19Z

.../src/test/java/org/elasticsearch/xpack/ml/datafeed/extractor/fields/ExtractedFieldTests.java

@@ -98,13 +96,6 @@ public void testNewTimeFieldGivenSource() {
        expectThrows(IllegalArgumentException.class, () -> ExtractedField.newTimeField("time", ExtractedField.ExtractionMethod.SOURCE));
    }

-    public void testValueGivenTimeField() {
-        final long millis = randomLong();


was this intentionally removed and not updated to ZonedDateTime?

the only values expected to be returned from searchHit are of types Long and String.
there is a separate testValueGivenStringTimeField but I think it wouldn't do any harm to add another testcase for testing Long.
The one for ZonedDateTime/DateTime is not needed

pgomulka · 2018-11-28T10:01:21Z

...ml/src/main/java/org/elasticsearch/xpack/ml/job/retention/AbstractExpiredJobDataRemover.java

@@ -70,7 +69,7 @@ private void removeData(Iterator<Job> jobIterator, ActionListener<Boolean> liste
    }

    private long calcCutoffEpochMs(long retentionDays) {
-        long nowEpochMs = DateTime.now(ISOChronology.getInstance()).getMillis();
+        long nowEpochMs = Instant.now().toEpochMilli();


This place is worthy to having a look too in regards to our comments about defaulted timezone

pgomulka added the WIP label Nov 27, 2018

pgomulka self-assigned this Nov 27, 2018

pgomulka force-pushed the java-time-ML branch 2 times, most recently from 1dddced to 68faca6 Compare November 27, 2018 15:11

pgomulka added :Core/Infra/Core Core issues without another label :ml Machine learning labels Nov 27, 2018

pgomulka force-pushed the java-time-ML branch 2 times, most recently from 0864968 to fce5c46 Compare November 27, 2018 15:27

pgomulka commented Nov 27, 2018

View reviewed changes

Core: Migrating from joda to java.time. ML package

502f230

part of the migrating joda time work. The goal is to find usages of joda time and refactor to use java.time. closes: elastic#35490

pgomulka force-pushed the java-time-ML branch from fce5c46 to 502f230 Compare November 27, 2018 16:05

pgomulka requested a review from spinscale November 27, 2018 23:37

spinscale suggested changes Nov 28, 2018

View reviewed changes

pgomulka commented Nov 28, 2018

View reviewed changes

pgomulka added 2 commits November 28, 2018 13:55

refactor parametless usages of now() to now(ZoneId)

3fdb2fe

unused import

efb49a8

spinscale approved these changes Nov 28, 2018

View reviewed changes

pgomulka merged commit 3a27760 into elastic:java-time Nov 28, 2018

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Migrating from joda to java.time. ML package #35949

Core: Migrating from joda to java.time. ML package #35949

pgomulka commented Nov 27, 2018 •

edited

Loading

elasticmachine commented Nov 27, 2018

elasticmachine commented Nov 27, 2018

pgomulka Nov 27, 2018

pgomulka Nov 28, 2018

pgomulka Nov 27, 2018 •

edited

Loading

spinscale Nov 28, 2018

spinscale left a comment

spinscale Nov 28, 2018

pgomulka Nov 28, 2018 •

edited

Loading

droberts195 Nov 28, 2018

spinscale Nov 28, 2018

spinscale Nov 28, 2018

pgomulka Nov 28, 2018

pgomulka Nov 28, 2018

droberts195 Nov 28, 2018

spinscale Nov 28, 2018

spinscale Nov 28, 2018

pgomulka Nov 28, 2018

pgomulka Nov 28, 2018

Core: Migrating from joda to java.time. ML package #35949

Core: Migrating from joda to java.time. ML package #35949

Conversation

pgomulka commented Nov 27, 2018 • edited Loading

elasticmachine commented Nov 27, 2018

elasticmachine commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spinscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgomulka Nov 28, 2018 • edited Loading

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

pgomulka commented Nov 27, 2018 •

edited

Loading

pgomulka Nov 27, 2018 •

edited

Loading

pgomulka Nov 28, 2018 •

edited

Loading