From 8c2a742c7f6bead3193d50d1f8939ab000c4cabd Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Fri, 20 Mar 2020 17:25:48 -0600 Subject: [PATCH 01/17] Disallow negative TimeValues This commit causes negative TimeValues, other than -1 which is sometimes used as a sentinel value, to be rejected during parsing. --- .../java/org/elasticsearch/common/unit/TimeValue.java | 7 ++++++- .../org/elasticsearch/common/unit/TimeValueTests.java | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java index edca86637e1b5..f0aeaf339db8d 100644 --- a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java +++ b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java @@ -386,7 +386,12 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St private static long parse(final String initialInput, final String normalized, final String suffix) { final String s = normalized.substring(0, normalized.length() - suffix.length()).trim(); try { - return Long.parseLong(s); + final long value = Long.parseLong(s); + if (value < -1) { + // -1 is magic, but reject any other negative values + throw new IllegalArgumentException("TimeValue does not support negative durations, but was given [" + initialInput + "]"); + } + return value; } catch (final NumberFormatException e) { try { @SuppressWarnings("unused") final double ignored = Double.parseDouble(s); diff --git a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java index 3dec990b7b251..ee7292a6bf8a6 100644 --- a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -238,4 +238,13 @@ public void testConversionHashCode() { TimeValue secondValue = new TimeValue(firstValue.getSeconds(), TimeUnit.SECONDS); assertEquals(firstValue.hashCode(), secondValue.hashCode()); } + + public void testRejectsNegativeValues() { + final long negativeValue = randomLongBetween(Long.MIN_VALUE, -2); + final String negativeTimeValueString = Long.toString(negativeValue) + randomTimeUnit(); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, + () -> TimeValue.parseTimeValue(negativeTimeValueString, "test-value")); + assertThat(ex.getMessage(), + equalTo("TimeValue does not support negative durations, but was given [" + negativeTimeValueString + "]")); + } } From cc2a587943c526e9620cc10ff7e024ae366f8cdb Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Fri, 20 Mar 2020 18:01:14 -0600 Subject: [PATCH 02/17] Adjust error message for negative TimeValues --- .../elasticsearch/common/unit/TimeValue.java | 23 ++++++++++--------- .../common/unit/TimeValueTests.java | 6 +++-- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java index f0aeaf339db8d..1c3b71b394a71 100644 --- a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java +++ b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java @@ -201,7 +201,7 @@ public double getDaysFrac() { * Returns a {@link String} representation of the current {@link TimeValue}. * * Note that this method might produce fractional time values (ex 1.6m) which cannot be - * parsed by method like {@link TimeValue#parse(String, String, String)}. + * parsed by method like {@link TimeValue#parse(String, String, String, String)}. * * Also note that the maximum string value that will be generated is * {@code 106751.9d} due to the way that values are internally converted @@ -216,7 +216,7 @@ public String toString() { * Returns a {@link String} representation of the current {@link TimeValue}. * * Note that this method might produce fractional time values (ex 1.6m) which cannot be - * parsed by method like {@link TimeValue#parse(String, String, String)}. The number of + * parsed by method like {@link TimeValue#parse(String, String, String, String)}. The number of * fractional decimals (up to 10 maximum) are truncated to the number of fraction pieces * specified. * @@ -358,20 +358,20 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St } final String normalized = sValue.toLowerCase(Locale.ROOT).trim(); if (normalized.endsWith("nanos")) { - return new TimeValue(parse(sValue, normalized, "nanos"), TimeUnit.NANOSECONDS); + return new TimeValue(parse(sValue, normalized, "nanos", settingName), TimeUnit.NANOSECONDS); } else if (normalized.endsWith("micros")) { - return new TimeValue(parse(sValue, normalized, "micros"), TimeUnit.MICROSECONDS); + return new TimeValue(parse(sValue, normalized, "micros", settingName), TimeUnit.MICROSECONDS); } else if (normalized.endsWith("ms")) { - return new TimeValue(parse(sValue, normalized, "ms"), TimeUnit.MILLISECONDS); + return new TimeValue(parse(sValue, normalized, "ms", settingName), TimeUnit.MILLISECONDS); } else if (normalized.endsWith("s")) { - return new TimeValue(parse(sValue, normalized, "s"), TimeUnit.SECONDS); + return new TimeValue(parse(sValue, normalized, "s", settingName), TimeUnit.SECONDS); } else if (sValue.endsWith("m")) { // parsing minutes should be case-sensitive as 'M' means "months", not "minutes"; this is the only special case. - return new TimeValue(parse(sValue, normalized, "m"), TimeUnit.MINUTES); + return new TimeValue(parse(sValue, normalized, "m", settingName), TimeUnit.MINUTES); } else if (normalized.endsWith("h")) { - return new TimeValue(parse(sValue, normalized, "h"), TimeUnit.HOURS); + return new TimeValue(parse(sValue, normalized, "h", settingName), TimeUnit.HOURS); } else if (normalized.endsWith("d")) { - return new TimeValue(parse(sValue, normalized, "d"), TimeUnit.DAYS); + return new TimeValue(parse(sValue, normalized, "d", settingName), TimeUnit.DAYS); } else if (normalized.matches("-0*1")) { return TimeValue.MINUS_ONE; } else if (normalized.matches("0+")) { @@ -383,13 +383,14 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St } } - private static long parse(final String initialInput, final String normalized, final String suffix) { + private static long parse(final String initialInput, final String normalized, final String suffix, String settingName) { final String s = normalized.substring(0, normalized.length() - suffix.length()).trim(); try { final long value = Long.parseLong(s); if (value < -1) { // -1 is magic, but reject any other negative values - throw new IllegalArgumentException("TimeValue does not support negative durations, but was given [" + initialInput + "]"); + throw new IllegalArgumentException("failed to parse setting [" + settingName + "] with value [" + initialInput + + "] as a time value: negative durations are not supported"); } return value; } catch (final NumberFormatException e) { diff --git a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java index ee7292a6bf8a6..ec82649d0f806 100644 --- a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -240,11 +240,13 @@ public void testConversionHashCode() { } public void testRejectsNegativeValues() { + final String settingName = "test-value"; final long negativeValue = randomLongBetween(Long.MIN_VALUE, -2); final String negativeTimeValueString = Long.toString(negativeValue) + randomTimeUnit(); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> TimeValue.parseTimeValue(negativeTimeValueString, "test-value")); + () -> TimeValue.parseTimeValue(negativeTimeValueString, settingName)); assertThat(ex.getMessage(), - equalTo("TimeValue does not support negative durations, but was given [" + negativeTimeValueString + "]")); + equalTo("failed to parse setting [" + settingName + "] with value [" + negativeTimeValueString + + "] as a time value: negative durations are not supported")); } } From 8b4535730d36fbc9612531926c3206bdc044ff0e Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Fri, 20 Mar 2020 18:02:11 -0600 Subject: [PATCH 03/17] Check for new message in tests for rejection of negative TimeValues --- .../elasticsearch/monitor/jvm/JvmGcMonitorService.java | 10 ++++------ .../indices/IndexingMemoryControllerTests.java | 6 ++++-- .../monitor/jvm/JvmGcMonitorServiceSettingsTests.java | 9 +++++---- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java b/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java index 70de50710c999..5744814293f07 100644 --- a/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java +++ b/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java @@ -28,8 +28,8 @@ import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.monitor.jvm.JvmStats.GarbageCollector; -import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.threadpool.Scheduler.Cancellable; +import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.threadpool.ThreadPool.Names; import java.util.HashMap; @@ -165,13 +165,11 @@ public JvmGcMonitorService(Settings settings, ThreadPool threadPool) { } private static TimeValue getValidThreshold(Settings settings, String key, String level) { - TimeValue threshold = settings.getAsTime(level, null); - if (threshold == null) { + final String thresholdString = settings.get(level, null); + if (thresholdString == null) { throw new IllegalArgumentException("missing gc_threshold for [" + getThresholdName(key, level) + "]"); } - if (threshold.nanos() <= 0) { - throw new IllegalArgumentException("invalid gc_threshold [" + threshold + "] for [" + getThresholdName(key, level) + "]"); - } + TimeValue threshold = TimeValue.parseTimeValue(thresholdString, null, getThresholdName(key, level)); return threshold; } diff --git a/server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java b/server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java index 9d7a77583f48d..663967dfbb447 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndexingMemoryControllerTests.java @@ -253,7 +253,8 @@ public void testNegativeInterval() { Exception e = expectThrows(IllegalArgumentException.class, () -> new MockController(Settings.builder() .put("indices.memory.interval", "-42s").build())); - assertEquals("failed to parse value [-42s] for setting [indices.memory.interval], must be >= [0ms]", e.getMessage()); + assertEquals("failed to parse setting [indices.memory.interval] with value " + + "[-42s] as a time value: negative durations are not supported", e.getMessage()); } @@ -261,7 +262,8 @@ public void testNegativeShardInactiveTime() { Exception e = expectThrows(IllegalArgumentException.class, () -> new MockController(Settings.builder() .put("indices.memory.shard_inactive_time", "-42s").build())); - assertEquals("failed to parse value [-42s] for setting [indices.memory.shard_inactive_time], must be >= [0ms]", e.getMessage()); + assertEquals("failed to parse setting [indices.memory.shard_inactive_time] with value " + + "[-42s] as a time value: negative durations are not supported", e.getMessage()); } diff --git a/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java b/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java index 5b60c82feb14f..7e090b252e620 100644 --- a/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java @@ -33,8 +33,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; -import static org.hamcrest.CoreMatchers.allOf; import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; public class JvmGcMonitorServiceSettingsTests extends ESTestCase { @@ -62,11 +62,12 @@ public void testDisabledSetting() throws InterruptedException { public void testNegativeSetting() throws InterruptedException { String collector = randomAlphaOfLength(5); - Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", "-" + randomTimeValue()).build(); + final String timeValue = "-" + randomTimeValue(); + Settings settings = Settings.builder().put("monitor.jvm.gc.collector." + collector + ".warn", timeValue).build(); execute(settings, (command, interval, name) -> null, e -> { assertThat(e, instanceOf(IllegalArgumentException.class)); - assertThat(e.getMessage(), allOf(containsString("invalid gc_threshold"), - containsString("for [monitor.jvm.gc.collector." + collector + "."))); + assertThat(e.getMessage(), equalTo("failed to parse setting [monitor.jvm.gc.collector." + collector + ".warn] " + + "with value [" + timeValue + "] as a time value: negative durations are not supported")); }, true, null); } From df20456abf576728d265e1beb4a2cd666f04065a Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 23 Mar 2020 17:22:30 -0600 Subject: [PATCH 04/17] Add workaround for ILM policies with negative min_age --- .../org/elasticsearch/xpack/core/ilm/Phase.java | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java index b2209d0956676..b4cf783f4e140 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.xpack.core.ilm; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; @@ -12,6 +14,7 @@ import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ConstructingObjectParser; +import org.elasticsearch.common.xcontent.ContextParser; import org.elasticsearch.common.xcontent.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -30,6 +33,7 @@ * particular point in the lifecycle of an index. */ public class Phase implements ToXContentObject, Writeable { + private static final Logger logger = LogManager.getLogger(Phase.class); public static final ParseField MIN_AGE = new ParseField("min_age"); public static final ParseField ACTIONS_FIELD = new ParseField("actions"); @@ -40,7 +44,18 @@ public class Phase implements ToXContentObject, Writeable { .collect(Collectors.toMap(LifecycleAction::getWriteableName, Function.identity())))); static { PARSER.declareField(ConstructingObjectParser.optionalConstructorArg(), - (p, c) -> TimeValue.parseTimeValue(p.text(), MIN_AGE.getPreferredName()), MIN_AGE, ValueType.VALUE); + (ContextParser) (p, c) -> { + // In earlier versions it was possible to create a Phase with a negative `min_age` which would then cause errors + // when the phase is read from the cluster state during startup (even before negative timevalues were strictly + // disallowed) so this is a hack to treat negative `min_age`s as 0 to prevent those errors. + final String timeValueString = p.text(); + if (timeValueString.startsWith("-")) { + logger.warn("phase has negative min_age value of [{}] - this will be treated as a min_age of 0", + timeValueString); + return TimeValue.ZERO; + } + return TimeValue.parseTimeValue(timeValueString, MIN_AGE.getPreferredName()); + }, MIN_AGE, ValueType.VALUE); PARSER.declareNamedObjects(ConstructingObjectParser.constructorArg(), (p, c, n) -> p.namedObject(LifecycleAction.class, n, null), v -> { throw new IllegalArgumentException("ordered " + ACTIONS_FIELD.getPreferredName() + " are not supported"); From af86c3df51abdbfbb8b853357dec279260ce98a2 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 23 Mar 2020 17:39:59 -0600 Subject: [PATCH 05/17] Add clarifying comment --- .../src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java index b4cf783f4e140..7c2df0d5f40c0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java @@ -48,6 +48,7 @@ public class Phase implements ToXContentObject, Writeable { // In earlier versions it was possible to create a Phase with a negative `min_age` which would then cause errors // when the phase is read from the cluster state during startup (even before negative timevalues were strictly // disallowed) so this is a hack to treat negative `min_age`s as 0 to prevent those errors. + // They will be saved as `0` so this hack can be removed once we no longer have to read cluster states from 7.x. final String timeValueString = p.text(); if (timeValueString.startsWith("-")) { logger.warn("phase has negative min_age value of [{}] - this will be treated as a min_age of 0", From 4f2636ac4427e3f4dbf5c2716dd93d73db069d0e Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 23 Mar 2020 18:06:32 -0600 Subject: [PATCH 06/17] Check for negative durations in constructor --- .../org/elasticsearch/common/unit/TimeValue.java | 3 +++ .../elasticsearch/common/unit/TimeValueTests.java | 12 +++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java index 1c3b71b394a71..b648e3f663f98 100644 --- a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java +++ b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java @@ -47,6 +47,9 @@ public TimeValue(long millis) { } public TimeValue(long duration, TimeUnit timeUnit) { + if (duration < -1) { + throw new IllegalArgumentException("durations cannot be negative, was given [" + duration + "]"); + } this.duration = duration; this.timeUnit = timeUnit; } diff --git a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java index ec82649d0f806..324522942abe1 100644 --- a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -239,7 +239,7 @@ public void testConversionHashCode() { assertEquals(firstValue.hashCode(), secondValue.hashCode()); } - public void testRejectsNegativeValues() { + public void testRejectsNegativeValuesDuringParsing() { final String settingName = "test-value"; final long negativeValue = randomLongBetween(Long.MIN_VALUE, -2); final String negativeTimeValueString = Long.toString(negativeValue) + randomTimeUnit(); @@ -249,4 +249,14 @@ public void testRejectsNegativeValues() { equalTo("failed to parse setting [" + settingName + "] with value [" + negativeTimeValueString + "] as a time value: negative durations are not supported")); } + + public void testRejectsNegativeValuesAtCreation() { + final long duration = randomLongBetween(Long.MIN_VALUE, -2); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new TimeValue(duration, randomTimeUnitObject())); + assertThat(ex.getMessage(), containsString("durations cannot be negative")); + } + + private TimeUnit randomTimeUnitObject() { + return randomFrom(TimeUnit.NANOSECONDS, TimeUnit.MICROSECONDS, TimeUnit.MILLISECONDS, TimeUnit.SECONDS, TimeUnit.MINUTES, TimeUnit.HOURS, TimeUnit.DAYS); + } } From 5baf1f5bf717e886f83dc6864e92a9432c5b9143 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 23 Mar 2020 18:14:35 -0600 Subject: [PATCH 07/17] Line length --- .../java/org/elasticsearch/common/unit/TimeValueTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java index 324522942abe1..c220681c14612 100644 --- a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -257,6 +257,7 @@ public void testRejectsNegativeValuesAtCreation() { } private TimeUnit randomTimeUnitObject() { - return randomFrom(TimeUnit.NANOSECONDS, TimeUnit.MICROSECONDS, TimeUnit.MILLISECONDS, TimeUnit.SECONDS, TimeUnit.MINUTES, TimeUnit.HOURS, TimeUnit.DAYS); + return randomFrom(TimeUnit.NANOSECONDS, TimeUnit.MICROSECONDS, TimeUnit.MILLISECONDS, TimeUnit.SECONDS, + TimeUnit.MINUTES, TimeUnit.HOURS, TimeUnit.DAYS); } } From df6900e8a079bc02ec53bae8c47f6ee910aab937 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Mon, 23 Mar 2020 20:39:42 -0600 Subject: [PATCH 08/17] Fix a giant pile of tests --- .../client/ccr/CcrStatsResponseTests.java | 2 +- .../IndexLifecycleExplainResponseTests.java | 2 +- .../index/search/stats/SearchStats.java | 15 ++++++++++--- .../index/shard/IndexingStats.java | 7 +++++- .../search/SearchResponseMergerTests.java | 8 +++---- .../elasticsearch/common/RoundingTests.java | 4 ++-- .../rounding/TimeZoneRoundingTests.java | 4 ++-- .../ShardFollowNodeTaskStatusTests.java | 2 +- .../xpack/ccr/action/StatsResponsesTests.java | 2 +- .../core/ml/datafeed/DatafeedConfigTests.java | 6 ++--- .../SnapshotRetentionConfigurationTests.java | 2 +- .../action/EnrichStatsResponseTests.java | 2 +- .../xpack/ilm/IndexLifecycleRunner.java | 14 +++++++++--- .../xpack/slm/SnapshotLifecycleTaskTests.java | 6 +++-- .../support/WatcherDateTimeUtilsTests.java | 22 ------------------- 15 files changed, 50 insertions(+), 48 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ccr/CcrStatsResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ccr/CcrStatsResponseTests.java index a687ea45f2023..6b4ef517577d5 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/ccr/CcrStatsResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/ccr/CcrStatsResponseTests.java @@ -109,7 +109,7 @@ static FollowStatsAction.StatsResponses createStatsResponse() { randomNonNegativeLong(), randomNonNegativeLong(), Collections.emptyNavigableMap(), - randomLong(), + randomNonNegativeLong(), randomBoolean() ? new ElasticsearchException("fatal error") : null); responses.add(new FollowStatsAction.StatsResponse(status)); } diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/IndexLifecycleExplainResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/IndexLifecycleExplainResponseTests.java index 0510f0a16f4ac..71354fa5ce75f 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/IndexLifecycleExplainResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/ilm/IndexLifecycleExplainResponseTests.java @@ -57,7 +57,7 @@ private static IndexLifecycleExplainResponse randomManagedIndexExplainResponse() boolean stepNull = randomBoolean(); return IndexLifecycleExplainResponse.newManagedIndexResponse(randomAlphaOfLength(10), randomAlphaOfLength(10), - randomBoolean() ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomLongBetween(0, System.currentTimeMillis()), stepNull ? null : randomAlphaOfLength(10), stepNull ? null : randomAlphaOfLength(10), stepNull ? null : randomAlphaOfLength(10), diff --git a/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java b/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java index 7dc3ae4785cff..abd0832450a5c 100644 --- a/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java +++ b/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java @@ -154,7 +154,10 @@ public long getFetchCount() { } public TimeValue getFetchTime() { - return new TimeValue(fetchTimeInMillis); + if (fetchTimeInMillis >= -1) { + return new TimeValue(fetchTimeInMillis); + } + return TimeValue.MINUS_ONE; } public long getFetchTimeInMillis() { @@ -170,7 +173,10 @@ public long getScrollCount() { } public TimeValue getScrollTime() { - return new TimeValue(scrollTimeInMillis); + if (scrollTimeInMillis >= -1) { + return new TimeValue(scrollTimeInMillis); + } + return TimeValue.MINUS_ONE; } public long getScrollTimeInMillis() { @@ -190,7 +196,10 @@ public long getSuggestTimeInMillis() { } public TimeValue getSuggestTime() { - return new TimeValue(suggestTimeInMillis); + if (suggestTimeInMillis >= -1) { + return new TimeValue(suggestTimeInMillis); + } + return TimeValue.MINUS_ONE; } public long getSuggestCurrent() { diff --git a/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java b/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java index 99a4bda651bfb..8aa3ead3ab1e3 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java @@ -133,7 +133,12 @@ public long getDeleteCount() { /** * The total amount of time spend on executing delete operations. */ - public TimeValue getDeleteTime() { return new TimeValue(deleteTimeInMillis); } + public TimeValue getDeleteTime() { + if (deleteTimeInMillis >= -1) { + return new TimeValue(deleteTimeInMillis); + } + return TimeValue.MINUS_ONE; + } /** * Returns the currently in-flight delete operations diff --git a/server/src/test/java/org/elasticsearch/action/search/SearchResponseMergerTests.java b/server/src/test/java/org/elasticsearch/action/search/SearchResponseMergerTests.java index df04549ddee88..5d5c56751d65c 100644 --- a/server/src/test/java/org/elasticsearch/action/search/SearchResponseMergerTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/SearchResponseMergerTests.java @@ -94,12 +94,12 @@ private void awaitResponsesAdded() throws InterruptedException { } public void testMergeTookInMillis() throws InterruptedException { - long currentRelativeTime = randomLong(); + long currentRelativeTime = randomNonNegativeLong(); SearchTimeProvider timeProvider = new SearchTimeProvider(randomLong(), 0, () -> currentRelativeTime); SearchResponseMerger merger = new SearchResponseMerger(randomIntBetween(0, 1000), randomIntBetween(0, 10000), SearchContext.TRACK_TOTAL_HITS_ACCURATE, timeProvider, emptyReduceContextBuilder()); for (int i = 0; i < numResponses; i++) { - SearchResponse searchResponse = new SearchResponse(InternalSearchResponse.empty(), null, 1, 1, 0, randomLong(), + SearchResponse searchResponse = new SearchResponse(InternalSearchResponse.empty(), null, 1, 1, 0, randomNonNegativeLong(), ShardSearchFailure.EMPTY_ARRAY, SearchResponseTests.randomClusters()); addResponse(merger, searchResponse); } @@ -399,7 +399,7 @@ public void testMergeAggs() throws InterruptedException { } public void testMergeSearchHits() throws InterruptedException { - final long currentRelativeTime = randomLong(); + final long currentRelativeTime = randomNonNegativeLong(); final SearchTimeProvider timeProvider = new SearchTimeProvider(randomLong(), 0, () -> currentRelativeTime); final int size = randomIntBetween(0, 100); final int from = size > 0 ? randomIntBetween(0, 100) : 0; @@ -557,7 +557,7 @@ public void testMergeSearchHits() throws InterruptedException { } public void testMergeNoResponsesAdded() { - long currentRelativeTime = randomLong(); + long currentRelativeTime = randomNonNegativeLong(); final SearchTimeProvider timeProvider = new SearchTimeProvider(randomLong(), 0, () -> currentRelativeTime); SearchResponseMerger merger = new SearchResponseMerger(0, 10, Integer.MAX_VALUE, timeProvider, emptyReduceContextBuilder()); SearchResponse.Clusters clusters = SearchResponseTests.randomClusters(); diff --git a/server/src/test/java/org/elasticsearch/common/RoundingTests.java b/server/src/test/java/org/elasticsearch/common/RoundingTests.java index 8e19bdaf5547a..64537b2b008b8 100644 --- a/server/src/test/java/org/elasticsearch/common/RoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/RoundingTests.java @@ -129,8 +129,8 @@ public void testDayRounding() { Rounding tzRounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH) .timeZone(ZoneOffset.ofHours(timezoneOffset)).build(); assertThat(tzRounding.round(0), equalTo(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis())); - assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(0L - TimeValue - .timeValueHours(timezoneOffset).millis())); + assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(TimeValue + .timeValueHours(-timezoneOffset).millis())); ZoneId tz = ZoneId.of("-08:00"); tzRounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH).timeZone(tz).build(); diff --git a/server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java b/server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java index 029eb3b041d3d..c28ed44aefa72 100644 --- a/server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java +++ b/server/src/test/java/org/elasticsearch/common/rounding/TimeZoneRoundingTests.java @@ -128,8 +128,8 @@ public void testDayRounding() { Rounding tzRounding = Rounding.builder(DateTimeUnit.DAY_OF_MONTH).timeZone(DateTimeZone.forOffsetHours(timezoneOffset)) .build(); assertThat(tzRounding.round(0), equalTo(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis())); - assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(0L - TimeValue - .timeValueHours(timezoneOffset).millis())); + assertThat(tzRounding.nextRoundingValue(0L - TimeValue.timeValueHours(24 + timezoneOffset).millis()), equalTo(TimeValue + .timeValueHours(-timezoneOffset).millis())); DateTimeZone tz = DateTimeZone.forID("-08:00"); tzRounding = Rounding.builder(DateTimeUnit.DAY_OF_MONTH).timeZone(tz).build(); diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java index 413960b69c834..7b56b00cd6260 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java @@ -61,7 +61,7 @@ protected ShardFollowNodeTaskStatus createTestInstance() { randomNonNegativeLong(), randomNonNegativeLong(), randomReadExceptions(), - randomLong(), + randomNonNegativeLong(), randomBoolean() ? new ElasticsearchException("fatal error") : null); } diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java index 4e9aadf8d82a8..3707b5ab4542d 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/StatsResponsesTests.java @@ -59,7 +59,7 @@ static FollowStatsAction.StatsResponses createStatsResponse() { randomNonNegativeLong(), randomNonNegativeLong(), Collections.emptyNavigableMap(), - randomLong(), + randomNonNegativeLong(), randomBoolean() ? new ElasticsearchException("fatal error") : null); responses.add(new FollowStatsAction.StatsResponse(status)); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java index 659aa387f8c6d..a709df1ab5c79 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfigTests.java @@ -62,8 +62,8 @@ import java.util.Locale; import java.util.Map; -import static org.elasticsearch.xpack.core.ml.utils.QueryProviderTests.createRandomValidQueryProvider; import static org.elasticsearch.xpack.core.ml.job.messages.Messages.DATAFEED_AGGREGATIONS_INTERVAL_MUST_BE_GREATER_THAN_ZERO; +import static org.elasticsearch.xpack.core.ml.utils.QueryProviderTests.createRandomValidQueryProvider; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; @@ -492,8 +492,8 @@ public void testCheckValid_GivenIndicesContainsOnlyEmptyStrings() { public void testCheckValid_GivenNegativeQueryDelay() { DatafeedConfig.Builder conf = new DatafeedConfig.Builder("datafeed1", "job1"); IllegalArgumentException e = ESTestCase.expectThrows(IllegalArgumentException.class, - () -> conf.setQueryDelay(TimeValue.timeValueMillis(-10))); - assertEquals("query_delay cannot be less than 0. Value = -10", e.getMessage()); + () -> conf.setQueryDelay(TimeValue.timeValueMillis(-1))); + assertEquals("query_delay cannot be less than 0. Value = -1", e.getMessage()); } public void testCheckValid_GivenZeroFrequency() { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java index 430b076bab2c4..9659ca0fe2f54 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java @@ -182,7 +182,7 @@ public void testPartialsNotCountedTowardsMaximum() { } private void assertUnsuccessfulNotCountedTowardsMaximum(boolean failure) { - SnapshotRetentionConfiguration conf = new SnapshotRetentionConfiguration(() -> 1, TimeValue.timeValueDays(1), 2, 2); + SnapshotRetentionConfiguration conf = new SnapshotRetentionConfiguration(() -> 5, TimeValue.timeValueDays(1), 2, 2); SnapshotInfo s1 = makeInfo(1); SnapshotInfo s2 = makeFailureOrPartial(2, failure); SnapshotInfo s3 = makeFailureOrPartial(3, failure); diff --git a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/action/EnrichStatsResponseTests.java b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/action/EnrichStatsResponseTests.java index f0dbf5ef2ba39..a71ade28c4d0e 100644 --- a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/action/EnrichStatsResponseTests.java +++ b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/action/EnrichStatsResponseTests.java @@ -54,7 +54,7 @@ public static TaskInfo randomTaskInfo() { String action = randomAlphaOfLength(5); String description = randomAlphaOfLength(5); long startTime = randomLong(); - long runningTimeNanos = randomLong(); + long runningTimeNanos = randomNonNegativeLong(); boolean cancellable = randomBoolean(); TaskId parentTaskId = TaskId.EMPTY_TASK_ID; Map headers = randomBoolean() diff --git a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleRunner.java b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleRunner.java index 3d6ed82252ce4..5a436be695c77 100644 --- a/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleRunner.java +++ b/x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleRunner.java @@ -93,14 +93,22 @@ boolean isReadyToTransitionToThisPhase(final String policy, final IndexMetaData } final TimeValue after = stepRegistry.getIndexAgeForPhase(policy, phase); final long now = nowSupplier.getAsLong(); - final TimeValue age = new TimeValue(now - lifecycleDate); + final long ageMillis = now - lifecycleDate; + final TimeValue age; + if (ageMillis >= 0) { + age = new TimeValue(ageMillis); + } else if (ageMillis == Long.MIN_VALUE) { + age = new TimeValue(Long.MAX_VALUE); + } else { + age = new TimeValue(-ageMillis); + } if (logger.isTraceEnabled()) { logger.trace("[{}] checking for index age to be at least [{}] before performing actions in " + - "the \"{}\" phase. Now: {}, lifecycle date: {}, age: [{}/{}s]", + "the \"{}\" phase. Now: {}, lifecycle date: {}, age: [{}{}/{}s]", indexMetaData.getIndex().getName(), after, phase, new TimeValue(now).seconds(), new TimeValue(lifecycleDate).seconds(), - age, age.seconds()); + ageMillis < 0 ? "-" : "", age, age.seconds()); } return now >= lifecycleDate + after.getMillis(); } diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTaskTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTaskTests.java index 5474602cdfda6..8f2e54981684d 100644 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTaskTests.java +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTaskTests.java @@ -236,13 +236,15 @@ public void testPartialFailureSnapshot() throws Exception { Boolean.parseBoolean((String) policy.getConfig().get("include_global_state")); assertThat(req.includeGlobalState(), equalTo(globalState)); + long startTime = randomNonNegativeLong(); + long endTime = randomLongBetween(startTime, Long.MAX_VALUE); return new CreateSnapshotResponse( new SnapshotInfo( new SnapshotId(req.snapshot(), "uuid"), Arrays.asList(req.indices()), - randomNonNegativeLong(), + startTime, "snapshot started", - randomNonNegativeLong(), + endTime, 3, Collections.singletonList( new SnapshotShardFailure("nodeId", new ShardId("index", "uuid", 0), "forced failure")), diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherDateTimeUtilsTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherDateTimeUtilsTests.java index 8c08c12a493da..67cf5153520e0 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherDateTimeUtilsTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/support/WatcherDateTimeUtilsTests.java @@ -89,28 +89,6 @@ public void testParseTimeValueString() throws Exception { assertThat(parsed.millis(), is(values.get(key).millis())); } - public void testParseTimeValueStringNegative() throws Exception { - int value = -1 * randomIntBetween(2, 200); - Map values = new HashMap<>(); - values.put(value + "s", TimeValue.timeValueSeconds(value)); - values.put(value + "m", TimeValue.timeValueMinutes(value)); - values.put(value + "h", TimeValue.timeValueHours(value)); - - String key = randomFrom(values.keySet().toArray(new String[values.size()])); - - XContentParser parser = createParser(jsonBuilder().startObject().field("value", key).endObject()); - parser.nextToken(); // start object - parser.nextToken(); // field name - parser.nextToken(); // value - - try { - WatcherDateTimeUtils.parseTimeValue(parser, "test"); - fail("Expected ElasticsearchParseException"); - } catch (ElasticsearchParseException e) { - assertThat(e.getMessage(), is("failed to parse time unit")); - } - } - public void testParseTimeValueNull() throws Exception { XContentParser parser = createParser(jsonBuilder().startObject().nullField("value").endObject()); parser.nextToken(); // start object From 222bd830d44f19132867f05e5b801a6de57362c9 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 24 Mar 2020 15:16:42 -0600 Subject: [PATCH 09/17] Version assertion --- .../src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java index 7c2df0d5f40c0..3324e3f35a5bf 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/Phase.java @@ -7,6 +7,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; @@ -49,6 +50,7 @@ public class Phase implements ToXContentObject, Writeable { // when the phase is read from the cluster state during startup (even before negative timevalues were strictly // disallowed) so this is a hack to treat negative `min_age`s as 0 to prevent those errors. // They will be saved as `0` so this hack can be removed once we no longer have to read cluster states from 7.x. + assert Version.CURRENT.major < 9 : "remove this hack now that we don't have to read 7.x cluster states"; final String timeValueString = p.text(); if (timeValueString.startsWith("-")) { logger.warn("phase has negative min_age value of [{}] - this will be treated as a min_age of 0", From bcd33b8022083f83c78297d291a752215a26c382 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 24 Mar 2020 16:15:46 -0600 Subject: [PATCH 10/17] Adjust setting parsing per review --- .../monitor/jvm/JvmGcMonitorService.java | 15 ++++++++++++--- .../jvm/JvmGcMonitorServiceSettingsTests.java | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java b/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java index 5744814293f07..c6a554e6374d7 100644 --- a/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java +++ b/server/src/main/java/org/elasticsearch/monitor/jvm/JvmGcMonitorService.java @@ -165,11 +165,20 @@ public JvmGcMonitorService(Settings settings, ThreadPool threadPool) { } private static TimeValue getValidThreshold(Settings settings, String key, String level) { - final String thresholdString = settings.get(level, null); - if (thresholdString == null) { + final TimeValue threshold; + + try { + threshold = settings.getAsTime(level, null); + } catch (RuntimeException ex) { + final String settingValue = settings.get(level); + throw new IllegalArgumentException("failed to parse setting [" + getThresholdName(key, level) + "] with value [" + + settingValue + "] as a time value", ex); + } + + if (threshold == null) { throw new IllegalArgumentException("missing gc_threshold for [" + getThresholdName(key, level) + "]"); } - TimeValue threshold = TimeValue.parseTimeValue(thresholdString, null, getThresholdName(key, level)); + return threshold; } diff --git a/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java b/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java index 7e090b252e620..4e4ae1cb664c0 100644 --- a/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/monitor/jvm/JvmGcMonitorServiceSettingsTests.java @@ -67,7 +67,7 @@ public void testNegativeSetting() throws InterruptedException { execute(settings, (command, interval, name) -> null, e -> { assertThat(e, instanceOf(IllegalArgumentException.class)); assertThat(e.getMessage(), equalTo("failed to parse setting [monitor.jvm.gc.collector." + collector + ".warn] " + - "with value [" + timeValue + "] as a time value: negative durations are not supported")); + "with value [" + timeValue + "] as a time value")); }, true, null); } From b9bfb7312532159cca4e6e2cb6ef0fd09aec0e21 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 24 Mar 2020 16:38:37 -0600 Subject: [PATCH 11/17] Copy/paste ExplainResponse test fix from client to core --- .../xpack/core/ilm/IndexLifecycleExplainResponseTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleExplainResponseTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleExplainResponseTests.java index 18302cc96ef8a..862c408e6d645 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleExplainResponseTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/IndexLifecycleExplainResponseTests.java @@ -47,7 +47,7 @@ private static IndexLifecycleExplainResponse randomManagedIndexExplainResponse() boolean stepNull = randomBoolean(); return IndexLifecycleExplainResponse.newManagedIndexResponse(randomAlphaOfLength(10), randomAlphaOfLength(10), - randomBoolean() ? null : randomNonNegativeLong(), + randomBoolean() ? null : randomLongBetween(0, System.currentTimeMillis()), stepNull ? null : randomAlphaOfLength(10), stepNull ? null : randomAlphaOfLength(10), stepNull ? null : randomAlphaOfLength(10), From 5567b7adc157c7e9eae9a3c67860634da888be84 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Tue, 24 Mar 2020 17:31:57 -0600 Subject: [PATCH 12/17] Avoid negative TimeValues during serialization of incomplete snapshots --- .../src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java index 48d00a619f450..e45a42b63480c 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotInfo.java @@ -526,7 +526,7 @@ public XContentBuilder toXContent(final XContentBuilder builder, final Params pa if (verbose || endTime != 0) { builder.field(END_TIME, DATE_TIME_FORMATTER.format(Instant.ofEpochMilli(endTime).atZone(ZoneOffset.UTC))); builder.field(END_TIME_IN_MILLIS, endTime); - builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(endTime - startTime)); + builder.humanReadableField(DURATION_IN_MILLIS, DURATION, new TimeValue(Math.max(0L, endTime - startTime))); } if (verbose || !shardFailures.isEmpty()) { builder.startArray(FAILURES); From 41c80954c00a6aa5d0334da5d0cfcf278e6cc6b6 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 25 Mar 2020 13:11:05 -0600 Subject: [PATCH 13/17] Copy/paste Enrich stats test fix from core to client --- .../org/elasticsearch/client/enrich/StatsResponseTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/enrich/StatsResponseTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/enrich/StatsResponseTests.java index aac22348abb6d..efda1ab5ed4ea 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/enrich/StatsResponseTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/enrich/StatsResponseTests.java @@ -87,7 +87,7 @@ private static TaskInfo randomTaskInfo() { String action = randomAlphaOfLength(5); String description = randomAlphaOfLength(5); long startTime = randomLong(); - long runningTimeNanos = randomLong(); + long runningTimeNanos = randomNonNegativeLong(); boolean cancellable = randomBoolean(); TaskId parentTaskId = TaskId.EMPTY_TASK_ID; Map headers = randomBoolean() ? From a29c6396937388314b3981ec7f3fd7fd4254471b Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 25 Mar 2020 15:17:05 -0600 Subject: [PATCH 14/17] Undo stats serialization changes --- .../index/search/stats/SearchStats.java | 15 +++------------ .../elasticsearch/index/shard/IndexingStats.java | 5 +---- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java b/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java index abd0832450a5c..7dc3ae4785cff 100644 --- a/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java +++ b/server/src/main/java/org/elasticsearch/index/search/stats/SearchStats.java @@ -154,10 +154,7 @@ public long getFetchCount() { } public TimeValue getFetchTime() { - if (fetchTimeInMillis >= -1) { - return new TimeValue(fetchTimeInMillis); - } - return TimeValue.MINUS_ONE; + return new TimeValue(fetchTimeInMillis); } public long getFetchTimeInMillis() { @@ -173,10 +170,7 @@ public long getScrollCount() { } public TimeValue getScrollTime() { - if (scrollTimeInMillis >= -1) { - return new TimeValue(scrollTimeInMillis); - } - return TimeValue.MINUS_ONE; + return new TimeValue(scrollTimeInMillis); } public long getScrollTimeInMillis() { @@ -196,10 +190,7 @@ public long getSuggestTimeInMillis() { } public TimeValue getSuggestTime() { - if (suggestTimeInMillis >= -1) { - return new TimeValue(suggestTimeInMillis); - } - return TimeValue.MINUS_ONE; + return new TimeValue(suggestTimeInMillis); } public long getSuggestCurrent() { diff --git a/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java b/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java index 8aa3ead3ab1e3..c04ea6aec935b 100644 --- a/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java +++ b/server/src/main/java/org/elasticsearch/index/shard/IndexingStats.java @@ -134,10 +134,7 @@ public long getDeleteCount() { * The total amount of time spend on executing delete operations. */ public TimeValue getDeleteTime() { - if (deleteTimeInMillis >= -1) { - return new TimeValue(deleteTimeInMillis); - } - return TimeValue.MINUS_ONE; + return new TimeValue(deleteTimeInMillis); } /** From 996b4a2c865a095baee1166956bb85bcfe78cdf6 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 25 Mar 2020 15:42:50 -0600 Subject: [PATCH 15/17] Replace unrealistic, breakage-causing -1s in test with 0s --- .../collector/indices/IndicesStatsMonitoringDocTests.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/indices/IndicesStatsMonitoringDocTests.java b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/indices/IndicesStatsMonitoringDocTests.java index e5f9e6f56af31..f27d48b014191 100644 --- a/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/indices/IndicesStatsMonitoringDocTests.java +++ b/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/indices/IndicesStatsMonitoringDocTests.java @@ -146,14 +146,14 @@ public void testToXContent() throws IOException { private CommonStats mockCommonStats() { final CommonStats commonStats = new CommonStats(CommonStatsFlags.ALL); - commonStats.getDocs().add(new DocsStats(1L, -1L, randomNonNegativeLong())); + commonStats.getDocs().add(new DocsStats(1L, 0L, randomNonNegativeLong())); commonStats.getStore().add(new StoreStats(2L)); - final IndexingStats.Stats indexingStats = new IndexingStats.Stats(3L, 4L, -1L, -1L, -1L, -1L, -1L, -1L, true, 5L); + final IndexingStats.Stats indexingStats = new IndexingStats.Stats(3L, 4L, 0L, 0L, 0L, 0L, 0L, 0L, true, 5L); commonStats.getIndexing().add(new IndexingStats(indexingStats)); - final SearchStats.Stats searchStats = new SearchStats.Stats(6L, 7L, -1L, -1L, -1L, -1L, -1L, -1L, -1L, -1L, -1L, -1L); - commonStats.getSearch().add(new SearchStats(searchStats, -1L, null)); + final SearchStats.Stats searchStats = new SearchStats.Stats(6L, 7L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L); + commonStats.getSearch().add(new SearchStats(searchStats, 0L, null)); return commonStats; } From e4ac3bd111571b06049ea00f1623f544474bd25b Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 25 Mar 2020 17:25:49 -0600 Subject: [PATCH 16/17] Plural->singular per review Co-Authored-By: Lee Hinman --- .../src/main/java/org/elasticsearch/common/unit/TimeValue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java index b648e3f663f98..b2f9128673eb4 100644 --- a/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java +++ b/libs/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java @@ -48,7 +48,7 @@ public TimeValue(long millis) { public TimeValue(long duration, TimeUnit timeUnit) { if (duration < -1) { - throw new IllegalArgumentException("durations cannot be negative, was given [" + duration + "]"); + throw new IllegalArgumentException("duration cannot be negative, was given [" + duration + "]"); } this.duration = duration; this.timeUnit = timeUnit; From f893e1750e499232c753a9db14769f77f3b90751 Mon Sep 17 00:00:00 2001 From: Gordon Brown Date: Wed, 25 Mar 2020 17:39:09 -0600 Subject: [PATCH 17/17] Singular->plural in test as well --- .../test/java/org/elasticsearch/common/unit/TimeValueTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java index c220681c14612..5ee32099115d9 100644 --- a/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -253,7 +253,7 @@ public void testRejectsNegativeValuesDuringParsing() { public void testRejectsNegativeValuesAtCreation() { final long duration = randomLongBetween(Long.MIN_VALUE, -2); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new TimeValue(duration, randomTimeUnitObject())); - assertThat(ex.getMessage(), containsString("durations cannot be negative")); + assertThat(ex.getMessage(), containsString("duration cannot be negative")); } private TimeUnit randomTimeUnitObject() {