Skip to content

Commit ba8ad9c

Browse files
committed
Fix calculation of age of pending tasks
This commit addresses a time unit conversion bug in calculating the age of a PrioritizedRunnable. The issue was an incorrect conversion from nanoseconds to milliseconds as instead the conversion was to microseconds. This leads to the timeInQueue metric for pending tasks to be off by three orders of magnitude.
1 parent a954e4e commit ba8ad9c

File tree

2 files changed

+55
-2
lines changed

2 files changed

+55
-2
lines changed

core/src/main/java/org/elasticsearch/common/util/concurrent/PrioritizedRunnable.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,29 +20,39 @@
2020

2121
import org.elasticsearch.common.Priority;
2222

23+
import java.util.concurrent.TimeUnit;
24+
import java.util.function.LongSupplier;
25+
2326
/**
2427
*
2528
*/
2629
public abstract class PrioritizedRunnable implements Runnable, Comparable<PrioritizedRunnable> {
2730

2831
private final Priority priority;
2932
private final long creationDate;
33+
private final LongSupplier relativeTimeProvider;
3034

3135
public static PrioritizedRunnable wrap(Runnable runnable, Priority priority) {
3236
return new Wrapped(runnable, priority);
3337
}
3438

3539
protected PrioritizedRunnable(Priority priority) {
40+
this(priority, System::nanoTime);
41+
}
42+
43+
// package visible for testing
44+
PrioritizedRunnable(Priority priority, LongSupplier relativeTimeProvider) {
3645
this.priority = priority;
37-
creationDate = System.nanoTime();
46+
this.creationDate = relativeTimeProvider.getAsLong();
47+
this.relativeTimeProvider = relativeTimeProvider;
3848
}
3949

4050
public long getCreationDateInNanos() {
4151
return creationDate;
4252
}
4353

4454
public long getAgeInMillis() {
45-
return Math.max(0, (System.nanoTime() - creationDate) / 1000);
55+
return TimeUnit.MILLISECONDS.convert(relativeTimeProvider.getAsLong() - creationDate, TimeUnit.NANOSECONDS);
4656
}
4757

4858
@Override
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.common.util.concurrent;
21+
22+
import org.elasticsearch.common.Priority;
23+
import org.elasticsearch.test.ESTestCase;
24+
25+
import java.util.concurrent.TimeUnit;
26+
import java.util.concurrent.atomic.AtomicLong;
27+
28+
public class PrioritizedRunnableTests extends ESTestCase {
29+
public void testGetAgeInMillis() throws Exception {
30+
AtomicLong time = new AtomicLong();
31+
32+
PrioritizedRunnable runnable = new PrioritizedRunnable(Priority.NORMAL, time::get) {
33+
@Override
34+
public void run() {
35+
36+
}
37+
};
38+
assertEquals(0, runnable.getAgeInMillis());
39+
int milliseconds = randomIntBetween(1, 256);
40+
time.addAndGet(TimeUnit.NANOSECONDS.convert(milliseconds, TimeUnit.MILLISECONDS));
41+
assertEquals(milliseconds, runnable.getAgeInMillis());
42+
}
43+
}

0 commit comments

Comments
 (0)