Skip to content

Commit a1f494a

Browse files
JeremyDahlgrenJeremyDahlgren
authored
Fix TimeValue comparisons for -1 duration (#133677)
-1 duration for TimeValue has special meaning and is the only negative value allowed in the constructor. When a TimeValue string is parsed, "-1" is converted into TimeValue.MINUS_ONE, which uses TimeUnit.MILLISECONDS. Round trip equality of TimeValue.timeValueSeconds(-1) to string form and back will fail since the time units will be different in compareTo(). This change updates TimeValue.toStringRep() to include the units for negative values if the units differ from milliseconds. The long term goal is to eliminate the use of -1 and units in the constructor, so that MINUS_ONE is the only available option to represent a -1 TimeValue.
1 parent b5120c5 commit a1f494a

File tree

6 files changed

+50
-49
lines changed

6 files changed

+50
-49
lines changed

‎libs/core/src/main/java/org/elasticsearch/core/TimeValue.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ private static String formatDecimal(double value, int fractionPieces) {
340340
}
341341

342342
public String getStringRep() {
343-
if (duration < 0) {
343+
if (duration < 0 && TimeUnit.MILLISECONDS == timeUnit) {
344344
return Long.toString(duration);
345345
}
346346
return switch (timeUnit) {

‎libs/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ public void testRoundTrip() {
109109
assertThat(TimeValue.parseTimeValue(s, null, "test").getStringRep(), equalTo(s));
110110
final TimeValue t = new TimeValue(randomIntBetween(1, 128), randomFrom(TimeUnit.values()));
111111
assertThat(TimeValue.parseTimeValue(t.getStringRep(), null, "test"), equalTo(t));
112+
assertThat(TimeValue.timeValueSeconds(-1), equalTo(TimeValue.parseTimeValue(TimeValue.timeValueSeconds(-1).getStringRep(), "foo")));
112113
}
113114

114115
private static final String FRACTIONAL_TIME_VALUES_ARE_NOT_SUPPORTED = "fractional time values are not supported";

‎server/src/main/java/org/elasticsearch/index/IndexingSlowLog.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -35,29 +35,29 @@ public final class IndexingSlowLog implements IndexingOperationListener {
3535
public static final String INDEX_INDEXING_SLOWLOG_PREFIX = "index.indexing.slowlog";
3636
public static final Setting<TimeValue> INDEX_INDEXING_SLOWLOG_THRESHOLD_INDEX_WARN_SETTING = Setting.timeSetting(
3737
INDEX_INDEXING_SLOWLOG_PREFIX + ".threshold.index.warn",
38-
TimeValue.timeValueNanos(-1),
39-
TimeValue.timeValueMillis(-1),
38+
TimeValue.MINUS_ONE,
39+
TimeValue.MINUS_ONE,
4040
Property.Dynamic,
4141
Property.IndexScope
4242
);
4343
public static final Setting<TimeValue> INDEX_INDEXING_SLOWLOG_THRESHOLD_INDEX_INFO_SETTING = Setting.timeSetting(
4444
INDEX_INDEXING_SLOWLOG_PREFIX + ".threshold.index.info",
45-
TimeValue.timeValueNanos(-1),
46-
TimeValue.timeValueMillis(-1),
45+
TimeValue.MINUS_ONE,
46+
TimeValue.MINUS_ONE,
4747
Property.Dynamic,
4848
Property.IndexScope
4949
);
5050
public static final Setting<TimeValue> INDEX_INDEXING_SLOWLOG_THRESHOLD_INDEX_DEBUG_SETTING = Setting.timeSetting(
5151
INDEX_INDEXING_SLOWLOG_PREFIX + ".threshold.index.debug",
52-
TimeValue.timeValueNanos(-1),
53-
TimeValue.timeValueMillis(-1),
52+
TimeValue.MINUS_ONE,
53+
TimeValue.MINUS_ONE,
5454
Property.Dynamic,
5555
Property.IndexScope
5656
);
5757
public static final Setting<TimeValue> INDEX_INDEXING_SLOWLOG_THRESHOLD_INDEX_TRACE_SETTING = Setting.timeSetting(
5858
INDEX_INDEXING_SLOWLOG_PREFIX + ".threshold.index.trace",
59-
TimeValue.timeValueNanos(-1),
60-
TimeValue.timeValueMillis(-1),
59+
TimeValue.MINUS_ONE,
60+
TimeValue.MINUS_ONE,
6161
Property.Dynamic,
6262
Property.IndexScope
6363
);

‎server/src/main/java/org/elasticsearch/index/SearchSlowLog.java‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -55,57 +55,57 @@ public final class SearchSlowLog implements SearchOperationListener {
5555
);
5656
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_QUERY_WARN_SETTING = Setting.timeSetting(
5757
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.query.warn",
58-
TimeValue.timeValueNanos(-1),
59-
TimeValue.timeValueMillis(-1),
58+
TimeValue.MINUS_ONE,
59+
TimeValue.MINUS_ONE,
6060
Property.Dynamic,
6161
Property.IndexScope
6262
);
6363
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_QUERY_INFO_SETTING = Setting.timeSetting(
6464
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.query.info",
65-
TimeValue.timeValueNanos(-1),
66-
TimeValue.timeValueMillis(-1),
65+
TimeValue.MINUS_ONE,
66+
TimeValue.MINUS_ONE,
6767
Property.Dynamic,
6868
Property.IndexScope
6969
);
7070
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_QUERY_DEBUG_SETTING = Setting.timeSetting(
7171
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.query.debug",
72-
TimeValue.timeValueNanos(-1),
73-
TimeValue.timeValueMillis(-1),
72+
TimeValue.MINUS_ONE,
73+
TimeValue.MINUS_ONE,
7474
Property.Dynamic,
7575
Property.IndexScope
7676
);
7777
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_QUERY_TRACE_SETTING = Setting.timeSetting(
7878
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.query.trace",
79-
TimeValue.timeValueNanos(-1),
80-
TimeValue.timeValueMillis(-1),
79+
TimeValue.MINUS_ONE,
80+
TimeValue.MINUS_ONE,
8181
Property.Dynamic,
8282
Property.IndexScope
8383
);
8484
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_FETCH_WARN_SETTING = Setting.timeSetting(
8585
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.fetch.warn",
86-
TimeValue.timeValueNanos(-1),
87-
TimeValue.timeValueMillis(-1),
86+
TimeValue.MINUS_ONE,
87+
TimeValue.MINUS_ONE,
8888
Property.Dynamic,
8989
Property.IndexScope
9090
);
9191
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_FETCH_INFO_SETTING = Setting.timeSetting(
9292
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.fetch.info",
93-
TimeValue.timeValueNanos(-1),
94-
TimeValue.timeValueMillis(-1),
93+
TimeValue.MINUS_ONE,
94+
TimeValue.MINUS_ONE,
9595
Property.Dynamic,
9696
Property.IndexScope
9797
);
9898
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_FETCH_DEBUG_SETTING = Setting.timeSetting(
9999
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.fetch.debug",
100-
TimeValue.timeValueNanos(-1),
101-
TimeValue.timeValueMillis(-1),
100+
TimeValue.MINUS_ONE,
101+
TimeValue.MINUS_ONE,
102102
Property.Dynamic,
103103
Property.IndexScope
104104
);
105105
public static final Setting<TimeValue> INDEX_SEARCH_SLOWLOG_THRESHOLD_FETCH_TRACE_SETTING = Setting.timeSetting(
106106
INDEX_SEARCH_SLOWLOG_PREFIX + ".threshold.fetch.trace",
107-
TimeValue.timeValueNanos(-1),
108-
TimeValue.timeValueMillis(-1),
107+
TimeValue.MINUS_ONE,
108+
TimeValue.MINUS_ONE,
109109
Property.Dynamic,
110110
Property.IndexScope
111111
);

‎server/src/test/java/org/elasticsearch/index/IndexingSlowLogTests.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -430,18 +430,18 @@ public void testSetLevels() {
430430

431431
metadata = newIndexMeta("index", Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, IndexVersion.current()).build());
432432
settings.updateIndexMetadata(metadata);
433-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexTraceThreshold());
434-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexDebugThreshold());
435-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexInfoThreshold());
436-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexWarnThreshold());
433+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexTraceThreshold());
434+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexDebugThreshold());
435+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexInfoThreshold());
436+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexWarnThreshold());
437437

438438
settings = new IndexSettings(metadata, Settings.EMPTY);
439439
log = new IndexingSlowLog(settings, mock(SlowLogFields.class));
440440

441-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexTraceThreshold());
442-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexDebugThreshold());
443-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexInfoThreshold());
444-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getIndexWarnThreshold());
441+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexTraceThreshold());
442+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexDebugThreshold());
443+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexInfoThreshold());
444+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getIndexWarnThreshold());
445445
try {
446446
settings.updateIndexMetadata(
447447
newIndexMeta(

‎server/src/test/java/org/elasticsearch/index/SearchSlowLogTests.java‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -349,18 +349,18 @@ public void testSetQueryLevels() {
349349

350350
metadata = newIndexMeta("index", Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, IndexVersion.current()).build());
351351
settings.updateIndexMetadata(metadata);
352-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryTraceThreshold());
353-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryDebugThreshold());
354-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryInfoThreshold());
355-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryWarnThreshold());
352+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryTraceThreshold());
353+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryDebugThreshold());
354+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryInfoThreshold());
355+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryWarnThreshold());
356356

357357
settings = new IndexSettings(metadata, Settings.EMPTY);
358358
log = new SearchSlowLog(settings, mock(SlowLogFields.class));
359359

360-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryTraceThreshold());
361-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryDebugThreshold());
362-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryInfoThreshold());
363-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getQueryWarnThreshold());
360+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryTraceThreshold());
361+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryDebugThreshold());
362+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryInfoThreshold());
363+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getQueryWarnThreshold());
364364
try {
365365
settings.updateIndexMetadata(
366366
newIndexMeta(
@@ -455,18 +455,18 @@ public void testSetFetchLevels() {
455455

456456
metadata = newIndexMeta("index", Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, IndexVersion.current()).build());
457457
settings.updateIndexMetadata(metadata);
458-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchTraceThreshold());
459-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchDebugThreshold());
460-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchInfoThreshold());
461-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchWarnThreshold());
458+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchTraceThreshold());
459+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchDebugThreshold());
460+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchInfoThreshold());
461+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchWarnThreshold());
462462

463463
settings = new IndexSettings(metadata, Settings.EMPTY);
464464
log = new SearchSlowLog(settings, mock(SlowLogFields.class));
465465

466-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchTraceThreshold());
467-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchDebugThreshold());
468-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchInfoThreshold());
469-
assertEquals(TimeValue.timeValueMillis(-1).nanos(), log.getFetchWarnThreshold());
466+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchTraceThreshold());
467+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchDebugThreshold());
468+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchInfoThreshold());
469+
assertEquals(TimeValue.MINUS_ONE.nanos(), log.getFetchWarnThreshold());
470470
try {
471471
settings.updateIndexMetadata(
472472
newIndexMeta(

0 commit comments

Comments
 (0)