Fix flaky test CacheStatsAPIIndicesRequestCacheIT.testNullLevels() (o…

…pensearch-project#13457) * Fix flaky test Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Initialize CommonStatsFlags with empty array for levels Signed-off-by: Peter Alfonsi <petealft@amazon.com> * Fixes tests using incorrect null levels Signed-off-by: Peter Alfonsi <petealft@amazon.com> * rerun Signed-off-by: Peter Alfonsi <petealft@amazon.com> --------- Signed-off-by: Peter Alfonsi <petealft@amazon.com> Co-authored-by: Peter Alfonsi <petealft@amazon.com>
peteralfonsi · Apr 30, 2024 · 5d61ac2 · 5d61ac2
1 parent 1f406db
commit 5d61ac2
Show file tree

Hide file tree

Showing 8 changed files with 29 additions and 21 deletions.
diff --git a/...ns/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java b/...ns/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java
@@ -829,15 +829,16 @@ public void testInvalidateWithDropDimensions() throws Exception {
 
             ICacheKey<String> keyToDrop = keysAdded.get(0);
 
-            ImmutableCacheStats snapshot = ehCacheDiskCachingTier.stats().getStatsForDimensionValues(keyToDrop.dimensions);
+            String[] levels = dimensionNames.toArray(new String[0]);
+            ImmutableCacheStats snapshot = ehCacheDiskCachingTier.stats(levels).getStatsForDimensionValues(keyToDrop.dimensions);
             assertNotNull(snapshot);
 
             keyToDrop.setDropStatsForDimensions(true);
             ehCacheDiskCachingTier.invalidate(keyToDrop);
 
             // Now assert the stats are gone for any key that has this combination of dimensions, but still there otherwise
             for (ICacheKey<String> keyAdded : keysAdded) {
-                snapshot = ehCacheDiskCachingTier.stats().getStatsForDimensionValues(keyAdded.dimensions);
+                snapshot = ehCacheDiskCachingTier.stats(levels).getStatsForDimensionValues(keyAdded.dimensions);
                 if (keyAdded.dimensions.equals(keyToDrop.dimensions)) {
                     assertNull(snapshot);
                 } else {

diff --git a/server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStatsFlags.java b/server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStatsFlags.java
@@ -66,7 +66,7 @@ public class CommonStatsFlags implements Writeable, Cloneable {
     private boolean includeOnlyTopIndexingPressureMetrics = false;
     // Used for metric CACHE_STATS, to determine which caches to report stats for
     private EnumSet<CacheType> includeCaches = EnumSet.noneOf(CacheType.class);
-    private String[] levels;
+    private String[] levels = new String[0];
 
     /**
      * @param flags flags to set. If no flags are supplied, default flags will be set.
@@ -139,7 +139,7 @@ public CommonStatsFlags all() {
         includeAllShardIndexingPressureTrackers = false;
         includeOnlyTopIndexingPressureMetrics = false;
         includeCaches = EnumSet.noneOf(CacheType.class);
-        levels = null;
+        levels = new String[0];
         return this;
     }
 
@@ -156,7 +156,7 @@ public CommonStatsFlags clear() {
         includeAllShardIndexingPressureTrackers = false;
         includeOnlyTopIndexingPressureMetrics = false;
         includeCaches = EnumSet.noneOf(CacheType.class);
-        levels = null;
+        levels = new String[0];
         return this;
     }
 

diff --git a/server/src/main/java/org/opensearch/common/cache/ICache.java b/server/src/main/java/org/opensearch/common/cache/ICache.java
@@ -45,12 +45,12 @@ public interface ICache<K, V> extends Closeable {
 
     void refresh();
 
-    // Return all stats without aggregation.
+    // Return total stats only
     default ImmutableCacheStatsHolder stats() {
         return stats(null);
     }
 
-    // Return stats aggregated by the provided levels. If levels is null, do not aggregate and return all stats.
+    // Return stats aggregated by the provided levels. If levels is null or an empty array, return total stats only.
     ImmutableCacheStatsHolder stats(String[] levels);
 
     /**

diff --git a/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java b/server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java
@@ -12,6 +12,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -170,7 +171,8 @@ private boolean internalIncrementHelper(
      */
     @Override
     public ImmutableCacheStatsHolder getImmutableCacheStatsHolder(String[] levels) {
-        return new ImmutableCacheStatsHolder(this.statsRoot, levels, dimensionNames, storeName);
+        String[] nonNullLevels = Objects.requireNonNullElseGet(levels, () -> new String[0]);
+        return new ImmutableCacheStatsHolder(this.statsRoot, nonNullLevels, dimensionNames, storeName);
     }
 
     @Override

diff --git a/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java b/server/src/main/java/org/opensearch/indices/IndicesRequestCache.java
@@ -826,8 +826,8 @@ long count() {
     /**
      * Returns the current cache stats. Pkg-private for testing.
      */
-    ImmutableCacheStatsHolder stats() {
-        return cache.stats();
+    ImmutableCacheStatsHolder stats(String[] levels) {
+        return cache.stats(levels);
     }
 
     int numRegisteredCloseListeners() { // for testing

diff --git a/server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java b/server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java
@@ -29,10 +29,11 @@ public class ImmutableCacheStatsHolderTests extends OpenSearchTestCase {
 
     public void testSerialization() throws Exception {
         List<String> dimensionNames = List.of("dim1", "dim2", "dim3");
+        String[] levels = dimensionNames.toArray(new String[0]);
         DefaultCacheStatsHolder statsHolder = new DefaultCacheStatsHolder(dimensionNames, storeName);
         Map<String, List<String>> usedDimensionValues = DefaultCacheStatsHolderTests.getUsedDimensionValues(statsHolder, 10);
         DefaultCacheStatsHolderTests.populateStats(statsHolder, usedDimensionValues, 100, 10);
-        ImmutableCacheStatsHolder stats = statsHolder.getImmutableCacheStatsHolder(null);
+        ImmutableCacheStatsHolder stats = statsHolder.getImmutableCacheStatsHolder(levels);
         assertNotEquals(0, stats.getStatsRoot().children.size());
 
         BytesStreamOutput os = new BytesStreamOutput();
@@ -57,19 +58,20 @@ public void testSerialization() throws Exception {
 
     public void testEquals() throws Exception {
         List<String> dimensionNames = List.of("dim1", "dim2", "dim3");
+        String[] levels = dimensionNames.toArray(new String[0]);
         DefaultCacheStatsHolder statsHolder = new DefaultCacheStatsHolder(dimensionNames, storeName);
         DefaultCacheStatsHolder differentStoreNameStatsHolder = new DefaultCacheStatsHolder(dimensionNames, "nonMatchingStoreName");
         DefaultCacheStatsHolder nonMatchingStatsHolder = new DefaultCacheStatsHolder(dimensionNames, storeName);
         Map<String, List<String>> usedDimensionValues = DefaultCacheStatsHolderTests.getUsedDimensionValues(statsHolder, 10);
         DefaultCacheStatsHolderTests.populateStats(List.of(statsHolder, differentStoreNameStatsHolder), usedDimensionValues, 100, 10);
         DefaultCacheStatsHolderTests.populateStats(nonMatchingStatsHolder, usedDimensionValues, 100, 10);
-        ImmutableCacheStatsHolder stats = statsHolder.getImmutableCacheStatsHolder(null);
+        ImmutableCacheStatsHolder stats = statsHolder.getImmutableCacheStatsHolder(levels);
 
-        ImmutableCacheStatsHolder secondStats = statsHolder.getImmutableCacheStatsHolder(null);
+        ImmutableCacheStatsHolder secondStats = statsHolder.getImmutableCacheStatsHolder(levels);
         assertEquals(stats, secondStats);
-        ImmutableCacheStatsHolder nonMatchingStats = nonMatchingStatsHolder.getImmutableCacheStatsHolder(null);
+        ImmutableCacheStatsHolder nonMatchingStats = nonMatchingStatsHolder.getImmutableCacheStatsHolder(levels);
         assertNotEquals(stats, nonMatchingStats);
-        ImmutableCacheStatsHolder differentStoreNameStats = differentStoreNameStatsHolder.getImmutableCacheStatsHolder(null);
+        ImmutableCacheStatsHolder differentStoreNameStats = differentStoreNameStatsHolder.getImmutableCacheStatsHolder(levels);
         assertNotEquals(stats, differentStoreNameStats);
     }
 

diff --git a/server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java b/server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java
@@ -145,16 +145,16 @@ public void testInvalidateWithDropDimensions() throws Exception {
         }
 
         ICacheKey<String> keyToDrop = keysAdded.get(0);
-
-        ImmutableCacheStats snapshot = cache.stats().getStatsForDimensionValues(keyToDrop.dimensions);
+        String[] levels = dimensionNames.toArray(new String[0]);
+        ImmutableCacheStats snapshot = cache.stats(levels).getStatsForDimensionValues(keyToDrop.dimensions);
         assertNotNull(snapshot);
 
         keyToDrop.setDropStatsForDimensions(true);
         cache.invalidate(keyToDrop);
 
         // Now assert the stats are gone for any key that has this combination of dimensions, but still there otherwise
         for (ICacheKey<String> keyAdded : keysAdded) {
-            snapshot = cache.stats().getStatsForDimensionValues(keyAdded.dimensions);
+            snapshot = cache.stats(levels).getStatsForDimensionValues(keyAdded.dimensions);
             if (keyAdded.dimensions.equals(keyToDrop.dimensions)) {
                 assertNull(snapshot);
             } else {

diff --git a/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java
@@ -89,7 +89,9 @@
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import static org.opensearch.indices.IndicesRequestCache.INDEX_DIMENSION_NAME;
 import static org.opensearch.indices.IndicesRequestCache.INDICES_REQUEST_CACHE_STALENESS_THRESHOLD_SETTING;
+import static org.opensearch.indices.IndicesRequestCache.SHARD_ID_DIMENSION_NAME;
 import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -799,6 +801,7 @@ private String getReaderCacheKeyId(DirectoryReader reader) {
 
     public void testClosingIndexWipesStats() throws Exception {
         IndicesService indicesService = getInstanceFromNode(IndicesService.class);
+        String[] levels = { INDEX_DIMENSION_NAME, SHARD_ID_DIMENSION_NAME };
         // Create two indices each with multiple shards
         int numShards = 3;
         Settings indexSettings = Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, numShards).build();
@@ -873,8 +876,8 @@ public void testClosingIndexWipesStats() throws Exception {
                 ShardId shardId = indexService.getShard(i).shardId();
                 List<String> dimensionValues = List.of(shardId.getIndexName(), shardId.toString());
                 initialDimensionValues.add(dimensionValues);
-                ImmutableCacheStatsHolder holder = cache.stats();
-                ImmutableCacheStats snapshot = cache.stats().getStatsForDimensionValues(dimensionValues);
+                ImmutableCacheStatsHolder holder = cache.stats(levels);
+                ImmutableCacheStats snapshot = cache.stats(levels).getStatsForDimensionValues(dimensionValues);
                 assertNotNull(snapshot);
                 // check the values are not empty by confirming entries != 0, this should always be true since the missed value is loaded
                 // into the cache
@@ -895,7 +898,7 @@ public void testClosingIndexWipesStats() throws Exception {
 
         // Now stats for the closed index should be gone
         for (List<String> dimensionValues : initialDimensionValues) {
-            ImmutableCacheStats snapshot = cache.stats().getStatsForDimensionValues(dimensionValues);
+            ImmutableCacheStats snapshot = cache.stats(levels).getStatsForDimensionValues(dimensionValues);
             if (dimensionValues.get(0).equals(indexToCloseName)) {
                 assertNull(snapshot);
             } else {