XPack: core/ccr/Security-cli migration to java-time #38415

pgomulka · 2019-02-05T12:08:33Z

part of the migrating joda time work.
refactoring x-pack plugins usages of joda to java-time
refers #27330

migraiton of joda-time to java-time

elasticmachine · 2019-02-05T12:08:44Z

Pinging @elastic/es-core-infra

pgomulka · 2019-02-05T12:13:55Z

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java

@@ -29,7 +29,8 @@
 public class ScheduledEventTests extends AbstractSerializingTestCase<ScheduledEvent> {

    public static ScheduledEvent createScheduledEvent(String calendarId) {
-        ZonedDateTime start = ZonedDateTime.ofInstant(Instant.ofEpochMilli(new DateTime(randomDateTimeZone()).getMillis()), ZoneOffset.UTC);
+        long millis = Clock.system(randomZone()).millis();// TODO we are converting to UTC anyway??
+        ZonedDateTime start = Instant.ofEpochMilli(millis).atZone(ZoneOffset.UTC);


not sure we need to do soo many convertions here. maybe using just 'ZonedDateTime.now(UTC)` ?

ack. can you check why we need random timezone here at all?

danielmitterdorfer

LGTM

spinscale

I left few comments to be sure this works. especially when in 'mixed mode' during rolling upgrades (I do not know if anything gets serialized over the write, but it might be worth checking)

spinscale · 2019-02-05T16:00:16Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupRequestTranslator.java

@@ -220,7 +220,7 @@
                rolledDateHisto.interval(source.interval());
            }

-            String timezone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
+            String timezone = source.timeZone() == null ? ZoneOffset.UTC.toString() : source.timeZone().toString();


does this get serialized over the wire somewhere? If so we may need to convert it using DateUtils?

this is read in ValuesSourceAggregationBuilder so I guess it is already taken care of?
with

if (in.getVersion().before(Version.V_7_0_0)) { timeZone = DateUtils.dateTimeZoneToZoneId(in.readOptionalTimeZone()); } else { timeZone = in.readOptionalZoneId(); }

spinscale · 2019-02-05T16:00:35Z

.../rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java

@@ -288,7 +288,7 @@ static QueryBuilder rewriteQuery(QueryBuilder builder, Set<RollupJobCaps> jobCap
            String fieldName = range.fieldName();
            // Many range queries don't include the timezone because the default is UTC, but the query
            // builder will return null so we need to set it here
-            String timeZone = range.timeZone() == null ? DateTimeZone.UTC.toString() : range.timeZone();
+            String timeZone = range.timeZone() == null ? ZoneOffset.UTC.toString() : range.timeZone();


same as above

That is read from RangeQueryBuilder
does not seem to be using DateUtils
timeZone = in.readOptionalZoneId();
do you think that should be fixed in server?

please open an issue for that, so we do not forget about it

spinscale · 2019-02-05T16:01:50Z

...plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/calendars/ScheduledEventTests.java

@@ -29,7 +29,8 @@
 public class ScheduledEventTests extends AbstractSerializingTestCase<ScheduledEvent> {

    public static ScheduledEvent createScheduledEvent(String calendarId) {
-        ZonedDateTime start = ZonedDateTime.ofInstant(Instant.ofEpochMilli(new DateTime(randomDateTimeZone()).getMillis()), ZoneOffset.UTC);
+        long millis = Clock.system(randomZone()).millis();// TODO we are converting to UTC anyway??
+        ZonedDateTime start = Instant.ofEpochMilli(millis).atZone(ZoneOffset.UTC);


ack. can you check why we need random timezone here at all?

dimitris-athanasiou · 2019-02-05T16:35:13Z

Looks good for the ML changes.

pgomulka · 2019-02-05T18:50:15Z

this is failing the build because of change in RollupRequestTranslator.java
String timezone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
to this
String timezone = source.timeZone() == null ? ZoneOffset.UTC.toString() : source.timeZone().toString();
This should be fixed in https://github.com/elastic/elasticsearch/pull/36237/files and the string comparison won't be necessary

pgomulka · 2019-02-05T19:34:11Z

@elasticmachine run elasticsearch-ci/1

polyfractal · 2019-02-05T21:57:40Z

Re: the rollup bits, I've updated #36237 and will try to get it merged soon

The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by elastic#38415 and the fix is identical to the one that was made in elastic#38405.

The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by #38415 and the fix is identical to the one that was made in #38405.

) The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by elastic#38415 and the fix is identical to the one that was made in elastic#38405.

The clock resolution changed from jdk8 to jdk10, hence the test is passing in jdk8 but failing in jdk10. Scheduled events are serialised and deserialised with millisecond precision, making test fail in jdk 10 and higher. Fixes a problem introduced by #38415 and the fix is identical to the one that was made in #38405. Backport of #38506

* master: Add an authentication cache for API keys (elastic#38469) Fix exit code in certutil packaging test (elastic#38393) Enable logs for intermittent test failure (elastic#38426) Disable BWC to backport recovering retention leases (elastic#38477) Enable bwc tests now that elastic#38443 is backported. (elastic#38462) Fix Master Failover and DataNode Leave Blocking Snapshot (elastic#38460) Recover retention leases during peer recovery (elastic#38435) Set update mappings mater node timeout to 30 min (elastic#38439) Assert job is not null in FullClusterRestartIT (elastic#38218) Update ilm-api.asciidoc, point to REMOVE policy (elastic#38235) (elastic#38463) SQL: Fix esType for DATETIME/DATE and INTERVALS (elastic#38179) Handle deprecation header-AbstractUpgradeTestCase (elastic#38396) XPack: core/ccr/Security-cli migration to java-time (elastic#38415) Disable bwc tests for elastic#38443 (elastic#38456) Bubble-up exceptions from scheduler (elastic#38317) Re-enable TasksClientDocumentationIT.testCancelTasks (elastic#38234) Allow custom authorization with an authorization engine (elastic#38358) CRUDDocumentationIT fix documentation references Remove support for internal versioning for concurrency control (elastic#38254)

XPack: core/rollup/ccr/Security-cli migration to java-time

2d3bf55

migraiton of joda-time to java-time

pgomulka self-assigned this Feb 5, 2019

pgomulka added the :Core/Infra/Core Core issues without another label label Feb 5, 2019

pgomulka commented Feb 5, 2019

View reviewed changes

pgomulka added 2 commits February 5, 2019 15:31

Merge branch 'master' into feature/x_pack_jdk_migration

375ce19

Merge branch 'master' into feature/x_pack_jdk_migration

0ba2cf6

danielmitterdorfer approved these changes Feb 5, 2019

View reviewed changes

spinscale reviewed Feb 5, 2019

View reviewed changes

cleanup ml test

3ebfbad

spinscale approved these changes Feb 5, 2019

View reviewed changes

unused import

f4ad47d

pgomulka mentioned this pull request Feb 5, 2019

Deserialize timeZone with DateUtils in RangeQueryBuilder #38449

Closed

Merge branch 'master' into feature/x_pack_jdk_migration

82a21f9

revert rollback changes

edf9204

pgomulka changed the title ~~XPack: core/rollup/ccr/Security-cli migration to java-time~~ XPack: core/ccr/Security-cli migration to java-time Feb 5, 2019

pgomulka merged commit afcdbd2 into elastic:master Feb 5, 2019

droberts195 mentioned this pull request Feb 6, 2019

Fix the clock resolution to millis in ScheduledEventTests #38506

Merged

droberts195 mentioned this pull request Feb 6, 2019

Fix the clock resolution to millis in ScheduledEventTests #38516

Closed

droberts195 mentioned this pull request Feb 6, 2019

Fix the clock resolution to millis in ScheduledEventTests #38517

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XPack: core/ccr/Security-cli migration to java-time #38415

XPack: core/ccr/Security-cli migration to java-time #38415

pgomulka commented Feb 5, 2019

elasticmachine commented Feb 5, 2019

pgomulka Feb 5, 2019

spinscale Feb 5, 2019

danielmitterdorfer left a comment

spinscale left a comment

spinscale Feb 5, 2019

pgomulka Feb 5, 2019

spinscale Feb 5, 2019

spinscale Feb 5, 2019

pgomulka Feb 5, 2019

spinscale Feb 5, 2019

spinscale Feb 5, 2019

dimitris-athanasiou commented Feb 5, 2019

pgomulka commented Feb 5, 2019 •

edited

Loading

pgomulka commented Feb 5, 2019

polyfractal commented Feb 5, 2019

XPack: core/ccr/Security-cli migration to java-time #38415

XPack: core/ccr/Security-cli migration to java-time #38415

Conversation

pgomulka commented Feb 5, 2019

elasticmachine commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer left a 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

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

dimitris-athanasiou commented Feb 5, 2019

pgomulka commented Feb 5, 2019 • edited Loading

pgomulka commented Feb 5, 2019

polyfractal commented Feb 5, 2019

pgomulka commented Feb 5, 2019 •

edited

Loading