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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -13,10 +13,10 @@
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.xpack.core.monitoring.MonitoredSystem;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;

import java.io.IOException;
import java.time.Instant;
import java.time.ZoneOffset;
import java.util.Objects;

/**
Expand Down Expand Up @@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should work fine - the pattern is the same

//TODO that would break the formatting
return Instant.ofEpochMilli(timestamp).atZone(ZoneOffset.UTC).toString();
}

/**
Expand Down Expand Up @@ -250,4 +251,4 @@ public int hashCode() {
return Objects.hash(uuid, host, transportAddress, ip, name, timestamp);
}
}
}
}
Expand Up @@ -6,15 +6,16 @@
package org.elasticsearch.xpack.core.monitoring.exporter;

import org.elasticsearch.Version;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.xpack.core.monitoring.MonitoredSystem;
import org.elasticsearch.xpack.core.template.TemplateUtils;
import org.joda.time.format.DateTimeFormatter;
import org.elasticsearch.common.Strings;

import java.io.IOException;
import java.time.Instant;
import java.util.Locale;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -252,12 +253,12 @@ public static XContentBuilder emptyPipeline(final XContentType type) {
/**
* Get the index name given a specific date format, a monitored system and a timestamp.
*
* @param formatter the {@link DateTimeFormatter} to use to compute the timestamped index name
* @param formatter the {@link DateFormatter} to use to compute the timestamped index name
* @param system the {@link MonitoredSystem} for which the index name is computed
* @param timestamp the timestamp value to use to compute the timestamped index name
* @return the index name as a @{link String}
*/
public static String indexName(final DateTimeFormatter formatter, final MonitoredSystem system, final long timestamp) {
return ".monitoring-" + system.getSystem() + "-" + TEMPLATE_VERSION + "-" + formatter.print(timestamp);
public static String indexName(final DateFormatter formatter, final MonitoredSystem system, final long timestamp) {
return ".monitoring-" + system.getSystem() + "-" + TEMPLATE_VERSION + "-" + formatter.format(Instant.ofEpochMilli(timestamp));
}
}
Expand Up @@ -31,7 +31,7 @@ public Authentication authentication() {
@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
if (out.getVersion().before(Version.V_6_6_0)) {
if (out.getVersion().before(Version.V_7_0_0)) {
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
User.writeTo(authentication.getUser(), out);
} else {
authentication.writeTo(out);
Expand All @@ -41,7 +41,7 @@ public void writeTo(StreamOutput out) throws IOException {
@Override
public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
if (in.getVersion().before(Version.V_6_6_0)) {
if (in.getVersion().before(Version.V_7_0_0)) {
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
final User user = User.readFrom(in);
final Authentication.RealmRef unknownRealm = new Authentication.RealmRef("__unknown", "__unknown", "__unknown");
authentication = new Authentication(user, unknownRealm, unknownRealm);
Expand Down
Expand Up @@ -17,9 +17,9 @@
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xpack.core.monitoring.MonitoringField;
import org.joda.time.DateTime;
import org.joda.time.chrono.ISOChronology;

import java.time.Clock;
import java.time.ZonedDateTime;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ScheduledFuture;
Expand Down Expand Up @@ -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())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

executorName(),
runnable);
logger.debug("cleaning service started");
}

Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clock.systemUTC()?

TimeValue delay = executionScheduler.nextExecutionDelay(start);

logger.debug("scheduling next execution in [{}] seconds", delay.seconds());
Expand Down Expand Up @@ -234,7 +236,7 @@ interface ExecutionScheduler {
* @param now the current time
* @return the delay in millis
*/
TimeValue nextExecutionDelay(DateTime now);
TimeValue nextExecutionDelay(ZonedDateTime now);
}

/**
Expand All @@ -243,14 +245,16 @@ interface ExecutionScheduler {
static class DefaultExecutionScheduler implements ExecutionScheduler {

@Override
public TimeValue nextExecutionDelay(DateTime now) {
public TimeValue nextExecutionDelay(ZonedDateTime now) {
// Runs at 01:00 AM today or the next day if it's too late
DateTime next = now.withTimeAtStartOfDay().plusHours(1);
ZonedDateTime next = now.toLocalDate()
.atStartOfDay(now.getZone())
.plusHours(1);
// 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
Expand Up @@ -11,10 +11,10 @@
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.license.XPackLicenseState;
import org.joda.time.format.DateTimeFormat;
import org.joda.time.format.DateTimeFormatter;

import java.time.ZoneOffset;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -113,11 +113,11 @@ public void close() {

protected abstract void doClose();

protected static DateTimeFormatter dateTimeFormatter(final Config config) {
protected static DateFormatter dateTimeFormatter(final Config config) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} catch (IllegalArgumentException e) {
throw new SettingsException("[" + INDEX_NAME_TIME_FORMAT_SETTING.getKey() + "] invalid index name time format: ["
+ format + "]", e);
Expand Down
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.client.RestClient;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
Expand All @@ -28,9 +29,9 @@
import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils;
import org.elasticsearch.xpack.monitoring.exporter.ExportBulk;
import org.elasticsearch.xpack.monitoring.exporter.ExportException;
import org.joda.time.format.DateTimeFormatter;

import java.io.IOException;
import java.time.format.DateTimeFormatter;
import java.util.Collection;
import java.util.Map;

Expand All @@ -54,15 +55,15 @@ class HttpExportBulk extends ExportBulk {
/**
* {@link DateTimeFormatter} used to resolve timestamped index name.
*/
private final DateTimeFormatter formatter;
private final DateFormatter formatter;

/**
* The bytes payload that represents the bulk body is created via {@link #doAdd(Collection)}.
*/
private byte[] payload = null;

HttpExportBulk(final String name, final RestClient client, final Map<String, String> parameters,
final DateTimeFormatter dateTimeFormatter, final ThreadContext threadContext) {
final DateFormatter dateTimeFormatter, final ThreadContext threadContext) {
super(name, threadContext);

this.client = client;
Expand Down
Expand Up @@ -30,6 +30,7 @@
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsException;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.util.set.Sets;
Expand All @@ -41,7 +42,6 @@
import org.elasticsearch.xpack.monitoring.exporter.ClusterAlertsUtil;
import org.elasticsearch.xpack.monitoring.exporter.ExportBulk;
import org.elasticsearch.xpack.monitoring.exporter.Exporter;
import org.joda.time.format.DateTimeFormatter;

import javax.net.ssl.SSLContext;
import java.util.ArrayList;
Expand Down Expand Up @@ -193,7 +193,7 @@ public class HttpExporter extends Exporter {
private final AtomicBoolean clusterAlertsAllowed = new AtomicBoolean(false);

private final ThreadContext threadContext;
private final DateTimeFormatter dateTimeFormatter;
private final DateFormatter dateTimeFormatter;

/**
* Create an {@link HttpExporter}.
Expand Down
Expand Up @@ -14,13 +14,13 @@
import org.elasticsearch.client.Client;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringDoc;
import org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils;
import org.elasticsearch.xpack.monitoring.exporter.ExportBulk;
import org.elasticsearch.xpack.monitoring.exporter.ExportException;
import org.joda.time.format.DateTimeFormatter;

import java.util.Arrays;
import java.util.Collection;
Expand All @@ -37,13 +37,13 @@ public class LocalBulk extends ExportBulk {

private final Logger logger;
private final Client client;
private final DateTimeFormatter formatter;
private final DateFormatter formatter;
private final boolean usePipeline;

private BulkRequestBuilder requestBuilder;


LocalBulk(String name, Logger logger, Client client, DateTimeFormatter dateTimeFormatter, boolean usePipeline) {
LocalBulk(String name, Logger logger, Client client, DateFormatter dateTimeFormatter, boolean usePipeline) {
super(name, client.threadPool().getThreadContext());
this.logger = logger;
this.client = client;
Expand Down
Expand Up @@ -28,6 +28,7 @@
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.XContentType;
Expand All @@ -51,10 +52,11 @@
import org.elasticsearch.xpack.monitoring.exporter.ClusterAlertsUtil;
import org.elasticsearch.xpack.monitoring.exporter.ExportBulk;
import org.elasticsearch.xpack.monitoring.exporter.Exporter;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.joda.time.format.DateTimeFormatter;

import java.time.Instant;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
Expand Down Expand Up @@ -89,7 +91,7 @@ public class LocalExporter extends Exporter implements ClusterStateListener, Cle
private final XPackLicenseState licenseState;
private final CleanerService cleanerService;
private final boolean useIngest;
private final DateTimeFormatter dateTimeFormatter;
private final DateFormatter dateTimeFormatter;
private final List<String> clusterAlertBlacklist;

private final AtomicReference<State> state = new AtomicReference<>(State.INITIALIZED);
Expand Down Expand Up @@ -489,12 +491,12 @@ public void onCleanUpIndices(TimeValue retention) {
if (clusterService.state().nodes().isLocalNodeElectedMaster()) {
// Reference date time will be compared to index.creation_date settings,
// that's why it must be in UTC
DateTime expiration = new DateTime(DateTimeZone.UTC).minus(retention.millis());
ZonedDateTime expiration = ZonedDateTime.now(ZoneOffset.UTC).minus(retention.millis(), ChronoUnit.MILLIS);
logger.debug("cleaning indices [expiration={}, retention={}]", expiration, retention);

ClusterState clusterState = clusterService.state();
if (clusterState != null) {
final long expirationTimeMillis = expiration.getMillis();
final long expirationTimeMillis = expiration.toInstant().toEpochMilli();
final long currentTimeMillis = System.currentTimeMillis();
final boolean cleanUpWatcherHistory = clusterService.getClusterSettings().get(CLEAN_WATCHER_HISTORY);

Expand Down Expand Up @@ -524,7 +526,7 @@ public void onCleanUpIndices(TimeValue retention) {
if (creationDate <= expirationTimeMillis) {
if (logger.isDebugEnabled()) {
logger.debug("detected expired index [name={}, created={}, expired={}]",
indexName, new DateTime(creationDate, DateTimeZone.UTC), expiration);
indexName, Instant.ofEpochMilli(creationDate).atZone(ZoneOffset.UTC), expiration);
}
indices.add(indexName);
}
Expand Down