From 5cb11afa8485a4a1ea457cb873fe356593eada3e Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 13 Jun 2024 14:33:12 -0700 Subject: [PATCH 1/3] Fix flaky TSC stats tests Signed-off-by: Peter Alfonsi --- .../tier/TieredSpilloverCacheStatsIT.java | 36 ++++++++++++++----- .../CacheStatsAPIIndicesRequestCacheIT.java | 7 ++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java index 537caccbac652..904ca45e7dc94 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java @@ -10,6 +10,7 @@ import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; +import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse; import org.opensearch.action.admin.indices.stats.CommonStatsFlags; import org.opensearch.action.search.SearchResponse; import org.opensearch.client.Client; @@ -20,6 +21,7 @@ import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.index.IndexSettings; import org.opensearch.index.cache.request.RequestCacheStats; import org.opensearch.index.query.QueryBuilders; import org.opensearch.indices.IndicesRequestCache; @@ -102,8 +104,8 @@ public void testIndicesLevelAggregation() throws Exception { ); for (ImmutableCacheStatsHolder statsHolder : List.of(allLevelsStatsHolder, indicesOnlyStatsHolder)) { - assertEquals(index1ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index1Name))); - assertEquals(index2ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index2Name))); + assertStatsEqual(index1ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index1Name))); + assertStatsEqual(index2ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index2Name))); } } @@ -139,7 +141,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnHeapIndex1AfterTest") ) ); - assertEquals( + assertStatsEqual( index1HeapExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index1Name, TIER_DIMENSION_VALUE_ON_HEAP)) ); @@ -153,7 +155,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnHeapIndex2AfterTest") ) ); - assertEquals( + assertStatsEqual( index2HeapExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index2Name, TIER_DIMENSION_VALUE_ON_HEAP)) ); @@ -167,7 +169,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnDiskIndex1AfterTest") ) ); - assertEquals( + assertStatsEqual( index1DiskExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index1Name, TIER_DIMENSION_VALUE_DISK)) ); @@ -181,7 +183,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnDiskIndex2AfterTest") ) ); - assertEquals( + assertStatsEqual( index2DiskExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index2Name, TIER_DIMENSION_VALUE_DISK)) ); @@ -218,7 +220,7 @@ public void testTierLevelAggregation() throws Exception { ) ); ImmutableCacheStats heapStats = tiersOnlyStatsHolder.getStatsForDimensionValues(List.of(TIER_DIMENSION_VALUE_ON_HEAP)); - assertEquals(totalHeapExpectedStats, heapStats); + assertStatsEqual(totalHeapExpectedStats, heapStats); ImmutableCacheStats totalDiskExpectedStats = returnNullIfAllZero( new ImmutableCacheStats( values.get("hitsOnDiskIndex1") + values.get("hitsOnDiskIndex2"), @@ -229,7 +231,7 @@ public void testTierLevelAggregation() throws Exception { ) ); ImmutableCacheStats diskStats = tiersOnlyStatsHolder.getStatsForDimensionValues(List.of(TIER_DIMENSION_VALUE_DISK)); - assertEquals(totalDiskExpectedStats, diskStats); + assertStatsEqual(totalDiskExpectedStats, diskStats); } public void testInvalidLevelsAreIgnored() throws Exception { @@ -322,7 +324,7 @@ public void testStatsMatchOldApi() throws Exception { ImmutableCacheStats totalStats = getNodeCacheStatsResult(client, List.of()).getTotalStats(); // Check the new stats API values are as expected - assertEquals( + assertStatsEqual( new ImmutableCacheStats(expectedHits, expectedMisses, 0, expectedEntries * singleSearchSize, expectedEntries), totalStats ); @@ -351,11 +353,15 @@ private void startIndex(Client client, String indexName) throws InterruptedExcep .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + // Disable index refreshing to avoid cache being invalidated mid-test + .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), TimeValue.timeValueMillis(-1)) .build() ) .get() ); indexRandom(true, client.prepareIndex(indexName).setSource("k", "hello")); + // Force merge the index to ensure there can be no background merges during the subsequent searches that would invalidate the cache + ForceMergeResponse forceMergeResponse = client.admin().indices().prepareForceMerge(indexName).setFlush(true).get(); ensureSearchable(indexName); } @@ -498,4 +504,16 @@ private static ImmutableCacheStatsHolder getNodeCacheStatsResult(Client client, NodeCacheStats ncs = nodeStatsResponse.getNodes().get(0).getNodeCacheStats(); return ncs.getStatsByCache(CacheType.INDICES_REQUEST_CACHE); } + + // Check each stat separately for more transparency if there's a failure + private void assertStatsEqual(ImmutableCacheStats expected, ImmutableCacheStats actual) { + if (expected == null && actual == null) { + return; + } + assertEquals(expected.getHits(), actual.getHits()); + assertEquals(expected.getMisses(), actual.getMisses()); + assertEquals(expected.getEvictions(), actual.getEvictions()); + assertEquals(expected.getSizeInBytes(), actual.getSizeInBytes()); + assertEquals(expected.getItems(), actual.getItems()); + } } diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/CacheStatsAPIIndicesRequestCacheIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/CacheStatsAPIIndicesRequestCacheIT.java index 0539f96e429c1..28bac3c7441b6 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/CacheStatsAPIIndicesRequestCacheIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/CacheStatsAPIIndicesRequestCacheIT.java @@ -13,6 +13,7 @@ import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; import org.opensearch.action.admin.indices.cache.clear.ClearIndicesCacheRequest; +import org.opensearch.action.admin.indices.forcemerge.ForceMergeResponse; import org.opensearch.action.admin.indices.stats.CommonStatsFlags; import org.opensearch.action.search.SearchResponse; import org.opensearch.client.Client; @@ -23,12 +24,14 @@ import org.opensearch.common.cache.stats.ImmutableCacheStats; import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder; import org.opensearch.common.settings.Settings; +import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.index.IndexSettings; import org.opensearch.index.cache.request.RequestCacheStats; import org.opensearch.index.query.QueryBuilders; import org.opensearch.test.OpenSearchIntegTestCase; @@ -266,10 +269,14 @@ private void startIndex(Client client, String indexName) throws InterruptedExcep .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true) .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + // Disable index refreshing to avoid cache being invalidated mid-test + .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), TimeValue.timeValueMillis(-1)) ) .get() ); indexRandom(true, client.prepareIndex(indexName).setSource("k", "hello")); + // Force merge the index to ensure there can be no background merges during the subsequent searches that would invalidate the cache + ForceMergeResponse forceMergeResponse = client.admin().indices().prepareForceMerge(indexName).setFlush(true).get(); ensureSearchable(indexName); } From 4d7d64d60561495359c8b422514a82724f1151cb Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 13 Jun 2024 17:56:26 -0700 Subject: [PATCH 2/3] Addressed andrross's comment Signed-off-by: Peter Alfonsi --- .../tier/TieredSpilloverCacheStatsIT.java | 30 ++++++------------- .../cache/stats/ImmutableCacheStats.java | 17 +++++++++++ 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java index 904ca45e7dc94..783b6083e9226 100644 --- a/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java +++ b/modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java @@ -104,8 +104,8 @@ public void testIndicesLevelAggregation() throws Exception { ); for (ImmutableCacheStatsHolder statsHolder : List.of(allLevelsStatsHolder, indicesOnlyStatsHolder)) { - assertStatsEqual(index1ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index1Name))); - assertStatsEqual(index2ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index2Name))); + assertEquals(index1ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index1Name))); + assertEquals(index2ExpectedStats, statsHolder.getStatsForDimensionValues(List.of(index2Name))); } } @@ -141,7 +141,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnHeapIndex1AfterTest") ) ); - assertStatsEqual( + assertEquals( index1HeapExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index1Name, TIER_DIMENSION_VALUE_ON_HEAP)) ); @@ -155,7 +155,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnHeapIndex2AfterTest") ) ); - assertStatsEqual( + assertEquals( index2HeapExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index2Name, TIER_DIMENSION_VALUE_ON_HEAP)) ); @@ -169,7 +169,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnDiskIndex1AfterTest") ) ); - assertStatsEqual( + assertEquals( index1DiskExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index1Name, TIER_DIMENSION_VALUE_DISK)) ); @@ -183,7 +183,7 @@ public void testIndicesAndTierLevelAggregation() throws Exception { values.get("itemsOnDiskIndex2AfterTest") ) ); - assertStatsEqual( + assertEquals( index2DiskExpectedStats, allLevelsStatsHolder.getStatsForDimensionValues(List.of(index2Name, TIER_DIMENSION_VALUE_DISK)) ); @@ -220,7 +220,7 @@ public void testTierLevelAggregation() throws Exception { ) ); ImmutableCacheStats heapStats = tiersOnlyStatsHolder.getStatsForDimensionValues(List.of(TIER_DIMENSION_VALUE_ON_HEAP)); - assertStatsEqual(totalHeapExpectedStats, heapStats); + assertEquals(totalHeapExpectedStats, heapStats); ImmutableCacheStats totalDiskExpectedStats = returnNullIfAllZero( new ImmutableCacheStats( values.get("hitsOnDiskIndex1") + values.get("hitsOnDiskIndex2"), @@ -231,7 +231,7 @@ public void testTierLevelAggregation() throws Exception { ) ); ImmutableCacheStats diskStats = tiersOnlyStatsHolder.getStatsForDimensionValues(List.of(TIER_DIMENSION_VALUE_DISK)); - assertStatsEqual(totalDiskExpectedStats, diskStats); + assertEquals(totalDiskExpectedStats, diskStats); } public void testInvalidLevelsAreIgnored() throws Exception { @@ -324,7 +324,7 @@ public void testStatsMatchOldApi() throws Exception { ImmutableCacheStats totalStats = getNodeCacheStatsResult(client, List.of()).getTotalStats(); // Check the new stats API values are as expected - assertStatsEqual( + assertEquals( new ImmutableCacheStats(expectedHits, expectedMisses, 0, expectedEntries * singleSearchSize, expectedEntries), totalStats ); @@ -504,16 +504,4 @@ private static ImmutableCacheStatsHolder getNodeCacheStatsResult(Client client, NodeCacheStats ncs = nodeStatsResponse.getNodes().get(0).getNodeCacheStats(); return ncs.getStatsByCache(CacheType.INDICES_REQUEST_CACHE); } - - // Check each stat separately for more transparency if there's a failure - private void assertStatsEqual(ImmutableCacheStats expected, ImmutableCacheStats actual) { - if (expected == null && actual == null) { - return; - } - assertEquals(expected.getHits(), actual.getHits()); - assertEquals(expected.getMisses(), actual.getMisses()); - assertEquals(expected.getEvictions(), actual.getEvictions()); - assertEquals(expected.getSizeInBytes(), actual.getSizeInBytes()); - assertEquals(expected.getItems(), actual.getItems()); - } } diff --git a/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java b/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java index dbd78a2584f9c..300f8a3277902 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java @@ -115,6 +115,23 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return builder; } + @Override + public String toString() { + return String.format( + "%s=%d, %s=%d, %s=%d, %s=%d, %s=%d", + Fields.HIT_COUNT, + hits, + Fields.MISS_COUNT, + misses, + Fields.EVICTIONS, + evictions, + Fields.SIZE_IN_BYTES, + sizeInBytes, + Fields.ITEM_COUNT, + items + ); + } + /** * Field names used to write the values in this object to XContent. */ From 3e5a124ba18a26fe1913b9e725e85e660735e193 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 13 Jun 2024 18:22:09 -0700 Subject: [PATCH 3/3] fix forbidden API Signed-off-by: Peter Alfonsi --- .../cache/stats/ImmutableCacheStats.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java b/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java index 300f8a3277902..db23e7b877596 100644 --- a/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java +++ b/server/src/main/java/org/opensearch/common/cache/stats/ImmutableCacheStats.java @@ -117,19 +117,25 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws @Override public String toString() { - return String.format( - "%s=%d, %s=%d, %s=%d, %s=%d, %s=%d", - Fields.HIT_COUNT, - hits, - Fields.MISS_COUNT, - misses, - Fields.EVICTIONS, - evictions, - Fields.SIZE_IN_BYTES, - sizeInBytes, - Fields.ITEM_COUNT, - items - ); + return Fields.HIT_COUNT + + "=" + + hits + + ", " + + Fields.MISS_COUNT + + "=" + + misses + + ", " + + Fields.EVICTIONS + + "=" + + evictions + + ", " + + Fields.SIZE_IN_BYTES + + "=" + + sizeInBytes + + ", " + + Fields.ITEM_COUNT + + "=" + + items; } /**