Skip to content

Commit

Permalink
[Tests] Make testEngineGCDeletesSetting deterministic (#38942)
Browse files Browse the repository at this point in the history
`InternalEngine.resolveDocVersion()` uses `relativeTimeInMillis()` from
`ThreadPool` so it needs, the cached time to be advanced. Add a check
to ensure that and decrease the `thread_pool.estimated_time_interval`
to 1msec to prevent long running times for the test.

Fixes: #38874

Co-authored-by: Boaz Leskes <b.leskes@gmail.com>
  • Loading branch information
2 people authored and dnhatn committed Mar 18, 2020
1 parent f0ff5fe commit 3f3033b
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 9 deletions.
23 changes: 19 additions & 4 deletions server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ public Collection<ExecutorBuilder> builders() {
}

public static Setting<TimeValue> ESTIMATED_TIME_INTERVAL_SETTING =
Setting.timeSetting("thread_pool.estimated_time_interval", TimeValue.timeValueMillis(200), Setting.Property.NodeScope);
Setting.timeSetting("thread_pool.estimated_time_interval",
TimeValue.timeValueMillis(200), TimeValue.ZERO, Setting.Property.NodeScope);

public ThreadPool(final Settings settings, final ExecutorBuilder<?>... customBuilders) {
assert Node.NODE_NAME_SETTING.exists(settings);
Expand Down Expand Up @@ -552,22 +553,36 @@ static class CachedTimeThread extends Thread {
/**
* Return the current time used for relative calculations. This is
* {@link System#nanoTime()} truncated to milliseconds.
* <p>
* If {@link ThreadPool#ESTIMATED_TIME_INTERVAL_SETTING} is set to 0
* then the cache is disabled and the method calls {@link System#nanoTime()}
* whenever called. Typically used for testing.
*/
long relativeTimeInMillis() {
return relativeMillis;
if (0 < interval) {
return relativeMillis;
}
return TimeValue.nsecToMSec(System.nanoTime());
}

/**
* Return the current epoch time, used to find absolute time. This is
* a cached version of {@link System#currentTimeMillis()}.
* <p>
* If {@link ThreadPool#ESTIMATED_TIME_INTERVAL_SETTING} is set to 0
* then the cache is disabled and the method calls {@link System#currentTimeMillis()}
* whenever called. Typically used for testing.
*/
long absoluteTimeInMillis() {
return absoluteMillis;
if (0 < interval) {
return absoluteMillis;
}
return System.currentTimeMillis();
}

@Override
public void run() {
while (running) {
while (running && 0 < interval) {
relativeMillis = TimeValue.nsecToMSec(System.nanoTime());
absoluteMillis = System.currentTimeMillis();
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.threadpool.ThreadPool;

import java.util.Arrays;
import java.util.Collection;
Expand All @@ -46,6 +47,7 @@
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertThrows;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.nullValue;

public class UpdateSettingsIT extends ESIntegTestCase {
Expand Down Expand Up @@ -125,6 +127,16 @@ public List<Setting<?>> getSettings() {
}
}

/**
* Needed by {@link UpdateSettingsIT#testEngineGCDeletesSetting()}
*/
@Override
protected Settings nodeSettings(int nodeOrdinal) {
return Settings.builder().put(super.nodeSettings(nodeOrdinal))
.put("thread_pool.estimated_time_interval", 0)
.build();
}

public void testUpdateDependentClusterSettings() {
IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () ->
client().admin().cluster().prepareUpdateSettings().setPersistentSettings(Settings.builder()
Expand Down Expand Up @@ -434,7 +446,7 @@ public void testOpenCloseUpdateSettings() throws Exception {
assertThat(getSettingsResponse.getSetting("test", "index.final"), nullValue());
}

public void testEngineGCDeletesSetting() throws InterruptedException {
public void testEngineGCDeletesSetting() throws Exception {
createIndex("test");
client().prepareIndex("test", "type", "1").setSource("f", 1).get(); // set version to 1
client().prepareDelete("test", "type", "1").get(); // sets version to 2
Expand All @@ -443,11 +455,16 @@ public void testEngineGCDeletesSetting() throws InterruptedException {
client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder().put("index.gc_deletes", 0)).get();

client().prepareDelete("test", "type", "1").get(); // sets version to 4
Thread.sleep(300); // wait for cache time to change TODO: this needs to be solved better. To be discussed.
// delete is should not be in cache
assertThrows(client().prepareIndex("test", "type", "1").setSource("f", 3)
.setVersion(4), VersionConflictEngineException.class);

// Make sure the time has advanced for InternalEngine#resolveDocVersion()
for (ThreadPool threadPool : internalCluster().getInstances(ThreadPool.class)) {
long startTime = threadPool.relativeTimeInMillis();
assertBusy(() -> assertThat(threadPool.relativeTimeInMillis(), greaterThan(startTime)));
}

// delete is should not be in cache
assertThrows(
client().prepareIndex("test", "type", "1").setSource("f", 3).setVersion(4), VersionConflictEngineException.class);
}

public void testUpdateSettingsWithBlocks() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@

package org.elasticsearch.threadpool;

import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;

import static org.elasticsearch.threadpool.ThreadPool.ESTIMATED_TIME_INTERVAL_SETTING;
import static org.hamcrest.CoreMatchers.equalTo;

public class ThreadPoolTests extends ESTestCase {
Expand Down Expand Up @@ -59,4 +61,10 @@ public void testAbsoluteTime() throws Exception {
threadPool.close();
}
}

public void testEstimatedTimeIntervalSettingAcceptsOnlyZeroAndPositiveTime() {
Settings settings = Settings.builder().put("thread_pool.estimated_time_interval", -1).build();
Exception e = expectThrows(IllegalArgumentException.class, () -> ESTIMATED_TIME_INTERVAL_SETTING.get(settings));
assertEquals("failed to parse value [-1] for setting [thread_pool.estimated_time_interval], must be >= [0ms]", e.getMessage());
}
}

0 comments on commit 3f3033b

Please sign in to comment.