From 58bde0ada1ec4df46368b8185e15aad184002dce Mon Sep 17 00:00:00 2001 From: Fabrizio Fortino Date: Thu, 12 Sep 2024 21:23:03 +0200 Subject: [PATCH] OAK-11072: (fix) flaky test: ElasticReliabilityTest.connectionCutOnQuery (#1710) * OAK-11072: (fix) flaky test: ElasticReliabilityTest.connectionCutOnQuery * OAK-11072: refactor ElasticIndexStatistics to allow cache properties changes when running it in a test suite --- .../index/elastic/ElasticIndexStatistics.java | 48 ++++++++++++------- .../elastic/ElasticIndexStatisticsTest.java | 12 ++++- .../index/elastic/ElasticReliabilityTest.java | 41 ++++++++++++---- 3 files changed, 73 insertions(+), 28 deletions(-) diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java index 07fdc54c70b..ed52c9b57d7 100644 --- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java +++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatistics.java @@ -56,15 +56,13 @@ * */ public class ElasticIndexStatistics implements IndexStatistics { - private static final Long MAX_SIZE = Long.getLong("oak.elastic.statsMaxSize", 10000); - private static final Long EXPIRE_SECONDS = Long.getLong("oak.elastic.statsExpireSeconds", 10 * 60); - private static final Long REFRESH_SECONDS = Long.getLong("oak.elastic.statsRefreshSeconds", 60); - private static final LoadingCache DEFAULT_COUNT_CACHE = - setupCountCache(MAX_SIZE, EXPIRE_SECONDS, REFRESH_SECONDS, null); - - private static final LoadingCache STATS_CACHE = - setupCache(MAX_SIZE, EXPIRE_SECONDS, REFRESH_SECONDS, new StatsCacheLoader(), null); + private static final String MAX_SIZE = "oak.elastic.statsMaxSize"; + private static final Long MAX_SIZE_DEFAULT = 10000L; + private static final String EXPIRE_SECONDS = "oak.elastic.statsExpireSeconds"; + private static final Long EXPIRE_SECONDS_DEFAULT = 10 * 60L; + private static final String REFRESH_SECONDS = "oak.elastic.statsRefreshSeconds"; + private static final Long REFRESH_SECONDS_DEFAULT = 60L; private static final ExecutorService REFRESH_EXECUTOR = new ThreadPoolExecutor( 0, 4, 60L, TimeUnit.SECONDS, @@ -78,19 +76,25 @@ public class ElasticIndexStatistics implements IndexStatistics { private final ElasticConnection elasticConnection; private final ElasticIndexDefinition indexDefinition; private final LoadingCache countCache; + private final LoadingCache statsCache; ElasticIndexStatistics(@NotNull ElasticConnection elasticConnection, @NotNull ElasticIndexDefinition indexDefinition) { - this(elasticConnection, indexDefinition, DEFAULT_COUNT_CACHE); + this(elasticConnection, indexDefinition, null, null); } @TestOnly ElasticIndexStatistics(@NotNull ElasticConnection elasticConnection, @NotNull ElasticIndexDefinition indexDefinition, - @NotNull LoadingCache countCache) { + @Nullable LoadingCache countCache, + @Nullable LoadingCache statsCache) { this.elasticConnection = elasticConnection; this.indexDefinition = indexDefinition; - this.countCache = countCache; + this.countCache = Objects.requireNonNullElseGet(countCache, () -> + setupCountCache(getCacheMaxSize(), getCacheExpireSeconds(), getCacheRefreshSeconds(), null)); + this.statsCache = Objects.requireNonNullElseGet(statsCache, () -> + setupCache(getCacheMaxSize(), getCacheExpireSeconds(), getCacheRefreshSeconds(), new StatsCacheLoader(), null)); + } /** @@ -127,7 +131,7 @@ public int getDocCountFor(Query query) { * {@code ElasticIndexDefinition}. */ public long primaryStoreSize() { - return STATS_CACHE.getUnchecked( + return statsCache.getUnchecked( new StatsRequestDescriptor(elasticConnection, indexDefinition.getIndexAlias()) ).primaryStoreSize; } @@ -137,7 +141,7 @@ public long primaryStoreSize() { * primary shards and replica shards. */ public long storeSize() { - return STATS_CACHE.getUnchecked( + return statsCache.getUnchecked( new StatsRequestDescriptor(elasticConnection, indexDefinition.getIndexAlias()) ).storeSize; } @@ -146,7 +150,7 @@ public long storeSize() { * Returns the creation date for the remote index bound to the {@code ElasticIndexDefinition}. */ public long creationDate() { - return STATS_CACHE.getUnchecked( + return statsCache.getUnchecked( new StatsRequestDescriptor(elasticConnection, indexDefinition.getIndexAlias()) ).creationDate; } @@ -156,7 +160,7 @@ public long creationDate() { * {@code ElasticIndexDefinition}. This document count includes hidden nested documents. */ public int luceneNumDocs() { - return STATS_CACHE.getUnchecked( + return statsCache.getUnchecked( new StatsRequestDescriptor(elasticConnection, indexDefinition.getIndexAlias()) ).luceneDocsCount; } @@ -166,7 +170,7 @@ public int luceneNumDocs() { * {@code ElasticIndexDefinition}. This document count includes hidden nested documents. */ public int luceneNumDeletedDocs() { - return STATS_CACHE.getUnchecked( + return statsCache.getUnchecked( new StatsRequestDescriptor(elasticConnection, indexDefinition.getIndexAlias()) ).luceneDocsDeleted; } @@ -188,6 +192,18 @@ static LoadingCache setupCache(long maxSize, long expireSeconds, lo return cacheBuilder.build(cacheLoader); } + private Long getCacheMaxSize() { + return Long.getLong(MAX_SIZE, MAX_SIZE_DEFAULT); + } + + private Long getCacheExpireSeconds() { + return Long.getLong(EXPIRE_SECONDS, EXPIRE_SECONDS_DEFAULT); + } + + private Long getCacheRefreshSeconds() { + return Long.getLong(REFRESH_SECONDS, REFRESH_SECONDS_DEFAULT); + } + static class CountCacheLoader extends CacheLoader { @Override diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatisticsTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatisticsTest.java index b6af3784679..aa2863f09c3 100644 --- a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatisticsTest.java +++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexStatisticsTest.java @@ -22,6 +22,7 @@ import co.elastic.clients.elasticsearch.core.CountResponse; import org.apache.jackrabbit.guava.common.base.Ticker; import org.apache.jackrabbit.guava.common.cache.LoadingCache; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -53,13 +54,20 @@ public class ElasticIndexStatisticsTest { @Mock private ElasticsearchClient elasticClientMock; + private AutoCloseable closeable; + @Before public void setUp() { - MockitoAnnotations.initMocks(this); + this.closeable = MockitoAnnotations.openMocks(this); when(indexDefinitionMock.getIndexAlias()).thenReturn("test-index"); when(elasticConnectionMock.getClient()).thenReturn(elasticClientMock); } + @After + public void releaseMocks() throws Exception { + closeable.close(); + } + @Test public void defaultIndexStatistics() { ElasticIndexStatistics indexStatistics = @@ -73,7 +81,7 @@ public void cachedStatistics() throws Exception { LoadingCache cache = ElasticIndexStatistics.setupCountCache(100, 10 * 60, 60, ticker); ElasticIndexStatistics indexStatistics = - new ElasticIndexStatistics(elasticConnectionMock, indexDefinitionMock, cache); + new ElasticIndexStatistics(elasticConnectionMock, indexDefinitionMock, cache, null); CountResponse countResponse = mock(CountResponse.class); when(countResponse.count()).thenReturn(100L); diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticReliabilityTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticReliabilityTest.java index 8bd434d06b6..b6b66b88937 100644 --- a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticReliabilityTest.java +++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticReliabilityTest.java @@ -22,13 +22,16 @@ import eu.rekawek.toxiproxy.model.toxic.LimitData; import org.apache.jackrabbit.oak.api.Tree; import org.junit.After; +import org.junit.Rule; import org.junit.Test; +import org.junit.contrib.java.lang.system.ProvideSystemProperty; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; import org.testcontainers.containers.ToxiproxyContainer; import org.testcontainers.utility.DockerImageName; import java.io.IOException; -import java.util.Arrays; -import java.util.Collections; +import java.util.List; +import java.util.Map; import java.util.UUID; import static org.hamcrest.CoreMatchers.containsString; @@ -37,6 +40,15 @@ public class ElasticReliabilityTest extends ElasticAbstractQueryTest { + // set cache expiration and refresh to low values to avoid cached results in tests + @Rule + public final ProvideSystemProperty updateSystemProperties + = new ProvideSystemProperty("oak.elastic.statsExpireSeconds", "5") + .and("oak.elastic.statsRefreshSeconds", "1"); + + @Rule + public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties(); + private static final DockerImageName TOXIPROXY_IMAGE = DockerImageName.parse("ghcr.io/shopify/toxiproxy:2.9.0"); private ToxiproxyContainer toxiproxy; @@ -82,25 +94,34 @@ public void connectionCutOnQuery() throws Exception { test.addChild("a").setProperty("propa", "a"); test.addChild("b").setProperty("propa", "c"); test.addChild("c").setProperty("propb", "e"); - root.commit(Collections.singletonMap("sync-mode", "rt")); + root.commit(Map.of("sync-mode", "rt")); String query = "select [jcr:path] from [nt:base] where propa is not null"; + assertEventually(() -> { + assertThat(explain(query), containsString("elasticsearch:" + indexName)); + assertQuery(query, List.of("/test/a", "/test/b")); + }); + // simulate an upstream connection cut LimitData cutConnectionUpstream = proxy.toxics() .limitData("CUT_CONNECTION_UPSTREAM", ToxicDirection.UPSTREAM, 0L); - // elastic is down, query should not use it - assertThat(explain(query), not(containsString("elasticsearch:" + indexName))); + assertEventually(() -> { + // elastic is down, query should not use it + assertThat(explain(query), not(containsString("elasticsearch:" + indexName))); - // result set should be correct anyway since traversal is enabled - assertQuery(query, Arrays.asList("/test/a", "/test/b")); + // result set should be correct anyway since traversal is enabled + assertQuery(query, List.of("/test/a", "/test/b")); + }); // re-establish connection cutConnectionUpstream.remove(); - // result set should be the same as before but this time elastic should be used - assertThat(explain(query), containsString("elasticsearch:" + indexName)); - assertQuery(query, Arrays.asList("/test/a", "/test/b")); + assertEventually(() -> { + // result set should be the same as before but this time elastic should be used + assertThat(explain(query), containsString("elasticsearch:" + indexName)); + assertQuery(query, List.of("/test/a", "/test/b")); + }); } }