Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Migrating from joda to java.time. Monitoring plugin #36297

Merged
merged 17 commits into from Feb 4, 2019

Conversation

Projects
None yet
5 participants
@pgomulka
Copy link
Contributor

commented Dec 6, 2018

monitoring plugin migration from joda to java.time

refers #27330

@pgomulka pgomulka self-assigned this Dec 6, 2018

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@pgomulka pgomulka force-pushed the pgomulka:feature/monitoring branch 5 times, most recently from 09de1e0 to 13846f5 Dec 6, 2018

@pgomulka pgomulka force-pushed the pgomulka:feature/monitoring branch 2 times, most recently from ee5a5d8 to 96109e1 Jan 25, 2019

@spinscale spinscale referenced this pull request Jan 25, 2019

Open

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

17 of 18 tasks complete

@pgomulka pgomulka force-pushed the pgomulka:feature/monitoring branch from 96109e1 to b1f2cc6 Jan 25, 2019

@pgomulka pgomulka force-pushed the pgomulka:feature/monitoring branch from b1f2cc6 to 93541a7 Jan 25, 2019

@@ -123,7 +123,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
* @return a string representing the timestamp
*/
public static String toUTC(final long timestamp) {
return new DateTime(timestamp, DateTimeZone.UTC).toString();

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

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?

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 31, 2019

Author Contributor

it should work fine - the pattern is the same

pgomulka added some commits Jan 28, 2019

@pgomulka 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 pgomulka removed the WIP label Jan 28, 2019

@danielmitterdorfer
Copy link
Member

left a comment

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

@pgomulka

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@elasticmachine run elasticsearch-ci/1

@pgomulka

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@elasticmachine run elasticsearch-ci/default-distro

@danielmitterdorfer
Copy link
Member

left a comment

LGTM.

@pgomulka

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

ok to test

@spinscale
Copy link
Member

left a comment

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

@@ -123,7 +123,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
* @return a string representing the timestamp
*/
public static String toUTC(final long timestamp) {
return new DateTime(timestamp, DateTimeZone.UTC).toString();

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

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?

@@ -57,7 +57,9 @@ public CleanerService(Settings settings, ClusterSettings clusterSettings, Thread
@Override
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())),

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

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

@@ -191,7 +193,7 @@ protected void doRunInLifecycle() throws Exception {
*/
@Override
protected void onAfterInLifecycle() {
DateTime start = new DateTime(ISOChronology.getInstance());
ZonedDateTime start = ZonedDateTime.now(Clock.systemDefaultZone());

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

Clock.systemUTC()?

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);

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

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

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);

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

does it make sense to make the formatter static?

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);

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

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

This comment has been minimized.

Copy link
@pgomulka

pgomulka Feb 1, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@spinscale

spinscale Feb 1, 2019

Member

indeed, nevermind.


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

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

week based formatter

@@ -105,7 +105,7 @@ public void testIndexName() {
assertThat(indexName(formatter, MonitoredSystem.BEATS, timestamp),
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);

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

week based formatter

@@ -352,7 +354,8 @@ public void testDynamicIndexFormatChange() throws Exception {
remoteClusterAllowsWatcher, currentLicenseAllowsWatcher, watcherAlreadyExists);
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);

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

week based formatter

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

This comment has been minimized.

Copy link
@spinscale

spinscale Jan 29, 2019

Member

week based formatter

@spinscale

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

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

// if it's not after now, then it needs to be the next day!
if (next.isAfter(now) == false) {
next = next.plusDays(1);
}
return TimeValue.timeValueMillis(next.getMillis() - now.getMillis());
return TimeValue.timeValueMillis(next.toInstant().toEpochMilli() - now.toInstant().toEpochMilli());

This comment has been minimized.

Copy link
@spinscale

spinscale Feb 4, 2019

Member

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

Setting<String> setting = INDEX_NAME_TIME_FORMAT_SETTING.getConcreteSettingForNamespace(config.name);
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 comment has been minimized.

Copy link
@spinscale

spinscale Feb 4, 2019

Member

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

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

FYI: I created #38309 for the required 6.7 change

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

7 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

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

dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Feb 12, 2019

Core: Migrating from joda to java.time. Monitoring plugin (elastic#36297
)

monitoring plugin migration from joda to java.time

refers elastic#27330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.