Skip to content

Commit

Permalink
Fix flaky test CacheStatsAPIIndicesRequestCacheIT.testNullLevels() (o…
Browse files Browse the repository at this point in the history 
…pensearch-project#13457)

* Fix flaky test

Signed-off-by: Peter Alfonsi <[email protected]>

* Initialize CommonStatsFlags with empty array for levels

Signed-off-by: Peter Alfonsi <[email protected]>

* Fixes tests using incorrect null levels

Signed-off-by: Peter Alfonsi <[email protected]>

* rerun

Signed-off-by: Peter Alfonsi <[email protected]>

---------

Signed-off-by: Peter Alfonsi <[email protected]>
Co-authored-by: Peter Alfonsi <[email protected]>
  • Loading branch information
@peteralfonsi @deshsidd
2 people authored and deshsidd committed May 17, 2024
1 parent 6c32719 commit b4da650
Show file tree
Hide file tree
Showing 8 changed files with 29 additions and 21 deletions.
5 changes: 3 additions & 2 deletions ...ns/cache-ehcache/src/test/java/org/opensearch/cache/store/disk/EhCacheDiskCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/opensearch/action/admin/indices/stats/CommonStatsFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -139,7 +139,7 @@ public CommonStatsFlags all() {
includeAllShardIndexingPressureTrackers = false;
includeOnlyTopIndexingPressureMetrics = false;
includeCaches = EnumSet.noneOf(CacheType.class);
levels = null;
levels = new String[0];
return this;
}

Expand All @@ -156,7 +156,7 @@ public CommonStatsFlags clear() {
includeAllShardIndexingPressureTrackers = false;
includeOnlyTopIndexingPressureMetrics = false;
includeCaches = EnumSet.noneOf(CacheType.class);
levels = null;
levels = new String[0];
return this;
}

Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/common/cache/ICache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

/**
Expand Down
4 changes: 3 additions & 1 deletion server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/opensearch/indices/IndicesRequestCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions server/src/test/java/org/opensearch/common/cache/stats/ImmutableCacheStatsHolderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
}

Expand Down
6 changes: 3 additions & 3 deletions server/src/test/java/org/opensearch/common/cache/store/OpenSearchOnHeapCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 6 additions & 3 deletions server/src/test/java/org/opensearch/indices/IndicesRequestCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down

0 comments on commit b4da650

Please sign in to comment.