Skip to content

Commit f85b719

Browse files
author
Peter Alfonsi
committed
Addressed Sagar's comments
Signed-off-by: Peter Alfonsi <[email protected]>
1 parent dc515d9 commit f85b719

File tree

7 files changed

+49
-48
lines changed

7 files changed

+49
-48
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
8282
- Fix warnings from SLF4J on startup when repository-s3 is installed ([#16194](https://github.com/opensearch-project/OpenSearch/pull/16194))
8383
- Fix protobuf-java leak through client library dependencies ([#16254](https://github.com/opensearch-project/OpenSearch/pull/16254))
8484
- Fix multi-search with template doesn't return status code ([#16265](https://github.com/opensearch-project/OpenSearch/pull/16265))
85-
- Fix bug in new cache stats API when closing shards while using TieredSpilloverCache ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560))
85+
- [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560))
8686

8787
### Security
8888

‎modules/cache-common/src/internalClusterTest/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsIT.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@
3838
import java.util.Map;
3939
import java.util.concurrent.TimeUnit;
4040

41-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME;
42-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
43-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
41+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME;
42+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
43+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
4444
import static org.opensearch.indices.IndicesService.INDICES_CACHE_CLEAN_INTERVAL_SETTING;
4545
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
4646
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse;

‎modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import org.opensearch.common.cache.RemovalReason;
2222
import org.opensearch.common.cache.policy.CachedQueryResult;
2323
import org.opensearch.common.cache.stats.ImmutableCacheStatsHolder;
24-
import org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder;
2524
import org.opensearch.common.cache.store.config.CacheConfig;
2625
import org.opensearch.common.collect.Tuple;
2726
import org.opensearch.common.settings.Setting;
@@ -54,10 +53,10 @@
5453
import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_DISK_STORE_SIZE;
5554
import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE;
5655
import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS;
56+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
57+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
5758
import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE;
5859
import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES;
59-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
60-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
6160

6261
/**
6362
* This cache spillover the evicted items from heap tier to disk tier. All the new items are first cached on heap

‎modules/cache-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java‎ renamed to ‎modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@
66
* compatible open source license.
77
*/
88

9-
package org.opensearch.common.cache.stats;
9+
package org.opensearch.cache.common.tier;
1010

11-
import org.opensearch.cache.common.tier.TieredSpilloverCache;
11+
import org.opensearch.common.cache.stats.DefaultCacheStatsHolder;
1212

1313
import java.util.ArrayList;
1414
import java.util.List;
@@ -177,20 +177,9 @@ public void removeDimensions(List<String> dimensionValues) {
177177
// As we are removing nodes from the tree, obtain the lock
178178
lock.lock();
179179
try {
180-
removeDimensionsHelper(dimensionValues, getStatsRoot(), 0);
180+
removeDimensionsHelper(dimensionValues, statsRoot, 0);
181181
} finally {
182182
lock.unlock();
183183
}
184184
}
185-
186-
@Override
187-
protected ImmutableCacheStats removeDimensionsBaseCase(Node node) {
188-
// The base case will be the node whose children represent individual tiers.
189-
// Manually delete this node's children
190-
for (String tierValue : TIER_VALUES) {
191-
node.children.remove(tierValue);
192-
}
193-
// Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations
194-
return node.getImmutableStats();
195-
}
196185
}

‎modules/cache-common/src/test/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolderTests.java‎ renamed to ‎modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolderTests.java‎

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@
66
* compatible open source license.
77
*/
88

9-
package org.opensearch.common.cache.stats;
9+
package org.opensearch.cache.common.tier;
1010

1111
import org.opensearch.common.Randomness;
12-
import org.opensearch.common.metrics.CounterMetric;
12+
import org.opensearch.common.cache.stats.CacheStats;
13+
import org.opensearch.common.cache.stats.DefaultCacheStatsHolder;
14+
import org.opensearch.common.cache.stats.ImmutableCacheStats;
1315
import org.opensearch.test.OpenSearchTestCase;
1416

1517
import java.util.ArrayList;
@@ -21,9 +23,9 @@
2123
import java.util.concurrent.ConcurrentHashMap;
2224
import java.util.concurrent.CountDownLatch;
2325

24-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
25-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
26-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_VALUES;
26+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
27+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
28+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_VALUES;
2729

2830
public class TieredSpilloverCacheStatsHolderTests extends OpenSearchTestCase {
2931
// These are modified from DefaultCacheStatsHolderTests.java to account for the tiers. Because we can't add a dependency on server.test,
@@ -78,12 +80,17 @@ public void testReset() throws Exception {
7880
cacheStatsHolder.reset();
7981
for (List<String> dimensionValues : expected.keySet()) {
8082
CacheStats originalCounter = expected.get(dimensionValues);
81-
originalCounter.sizeInBytes = new CounterMetric();
82-
originalCounter.items = new CounterMetric();
83+
ImmutableCacheStats expectedTotal = new ImmutableCacheStats(
84+
originalCounter.getHits(),
85+
originalCounter.getMisses(),
86+
originalCounter.getEvictions(),
87+
0,
88+
0
89+
);
8390

8491
DefaultCacheStatsHolder.Node node = getNode(dimensionValues, cacheStatsHolder.getStatsRoot());
8592
ImmutableCacheStats actual = node.getImmutableStats();
86-
assertEquals(originalCounter.immutableSnapshot(), actual);
93+
assertEquals(expectedTotal, actual);
8794
}
8895
}
8996

@@ -130,7 +137,7 @@ public void testDropStatsForDimensions() throws Exception {
130137
// When we invalidate the last node, all nodes should be deleted except the root node
131138
cacheStatsHolder.removeDimensions(List.of("A2", "B3"));
132139
assertEquals(new ImmutableCacheStats(0, 0, 0, 0, 0), cacheStatsHolder.getStatsRoot().getImmutableStats());
133-
assertEquals(0, cacheStatsHolder.getStatsRoot().children.size());
140+
// assertEquals(0, cacheStatsHolder.getStatsRoot().getChildren().size());
134141
}
135142
}
136143

@@ -280,11 +287,19 @@ static Map<List<String>, CacheStats> populateStats(
280287
threadRand.nextInt(5000),
281288
threadRand.nextInt(10)
282289
);
283-
expected.get(dimensions).hits.inc(statsToInc.getHits());
284-
expected.get(dimensions).misses.inc(statsToInc.getMisses());
285-
expected.get(dimensions).evictions.inc(statsToInc.getEvictions());
286-
expected.get(dimensions).sizeInBytes.inc(statsToInc.getSizeInBytes());
287-
expected.get(dimensions).items.inc(statsToInc.getItems());
290+
for (int iter = 0; iter < statsToInc.getHits(); iter++) {
291+
expected.get(dimensions).incrementHits();
292+
}
293+
for (int iter = 0; iter < statsToInc.getMisses(); iter++) {
294+
expected.get(dimensions).incrementMisses();
295+
}
296+
for (int iter = 0; iter < statsToInc.getEvictions(); iter++) {
297+
expected.get(dimensions).incrementEvictions();
298+
}
299+
expected.get(dimensions).incrementSizeInBytes(statsToInc.getSizeInBytes());
300+
for (int iter = 0; iter < statsToInc.getItems(); iter++) {
301+
expected.get(dimensions).incrementItems();
302+
}
288303
populateStatsHolderFromStatsValueMap(cacheStatsHolder, Map.of(dimensions, statsToInc), diskTierEnabled);
289304
}
290305
}

‎modules/cache-common/src/test/java/org/opensearch/cache/common/tier/TieredSpilloverCacheTests.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,11 @@
6161
import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_ONHEAP_STORE_SIZE;
6262
import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TIERED_SPILLOVER_SEGMENTS;
6363
import static org.opensearch.cache.common.tier.TieredSpilloverCacheSettings.TOOK_TIME_POLICY_CONCRETE_SETTINGS_MAP;
64+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME;
65+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
66+
import static org.opensearch.cache.common.tier.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
6467
import static org.opensearch.common.cache.settings.CacheSettings.INVALID_SEGMENT_COUNT_EXCEPTION_MESSAGE;
6568
import static org.opensearch.common.cache.settings.CacheSettings.VALID_SEGMENT_COUNT_VALUES;
66-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_NAME;
67-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_DISK;
68-
import static org.opensearch.common.cache.stats.TieredSpilloverCacheStatsHolder.TIER_DIMENSION_VALUE_ON_HEAP;
6969
import static org.opensearch.common.cache.store.settings.OpenSearchOnHeapCacheSettings.MAXIMUM_SIZE_IN_BYTES_KEY;
7070
import static org.mockito.ArgumentMatchers.any;
7171
import static org.mockito.Mockito.doThrow;

‎server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public class DefaultCacheStatsHolder implements CacheStatsHolder {
3737
// Non-leaf nodes have stats matching the sum of their children.
3838
// We use a tree structure, rather than a map with concatenated keys, to save on memory usage. If there are many leaf
3939
// nodes that share a parent, that parent's dimension value will only be stored once, not many times.
40-
private final Node statsRoot;
40+
protected final Node statsRoot;
4141
// To avoid sync problems, obtain a lock before creating or removing nodes in the stats tree.
4242
// No lock is needed to edit stats on existing nodes.
4343
protected final Lock lock = new ReentrantLock();
@@ -190,8 +190,10 @@ public void removeDimensions(List<String> dimensionValues) {
190190
// Returns a CacheStatsCounterSnapshot object for the stats to decrement if the removal happened, null otherwise.
191191
protected ImmutableCacheStats removeDimensionsHelper(List<String> dimensionValues, Node node, int depth) {
192192
if (depth == dimensionValues.size()) {
193+
// Remove children, if present.
194+
node.children.clear();
193195
// Pass up a snapshot of the original stats to avoid issues when the original is decremented by other fn invocations
194-
return removeDimensionsBaseCase(node);
196+
return node.getImmutableStats();
195197
}
196198
Node child = node.getChild(dimensionValues.get(depth));
197199
if (child == null) {
@@ -208,19 +210,15 @@ protected ImmutableCacheStats removeDimensionsHelper(List<String> dimensionValue
208210
return statsToDecrement;
209211
}
210212

211-
protected ImmutableCacheStats removeDimensionsBaseCase(Node node) {
212-
return node.getImmutableStats();
213-
}
214-
215213
// pkg-private for testing
216-
Node getStatsRoot() {
214+
public Node getStatsRoot() {
217215
return statsRoot;
218216
}
219217

220218
/**
221219
* Nodes that make up the tree in the stats holder.
222220
*/
223-
protected static class Node {
221+
public static class Node {
224222
private final String dimensionValue;
225223
// Map from dimensionValue to the DimensionNode for that dimension value.
226224
final Map<String, Node> children;
@@ -245,7 +243,7 @@ public String getDimensionValue() {
245243
return dimensionValue;
246244
}
247245

248-
protected Map<String, Node> getChildren() {
246+
public Map<String, Node> getChildren() {
249247
// We can safely iterate over ConcurrentHashMap without worrying about thread issues.
250248
return children;
251249
}
@@ -284,7 +282,7 @@ long getEntries() {
284282
return this.stats.getItems();
285283
}
286284

287-
ImmutableCacheStats getImmutableStats() {
285+
public ImmutableCacheStats getImmutableStats() {
288286
return this.stats.immutableSnapshot();
289287
}
290288

0 commit comments

Comments
 (0)