Skip to content

Commit

Permalink
Avoid negative memory result in IndicesQueryCache stats calculation (#…
Browse files Browse the repository at this point in the history 
…6965)

(#6917)

* Avoid negative memory result in IndicesQueryCache stats calculation

* Cache stores recent queries so isn't empty even with no doc id sets



* Add test which actually fails without the bug fix



---------

Signed-off-by: Daniel Widdis <[email protected]>
  • Loading branch information
@dbwiddis
dbwiddis authored Apr 12, 2023
1 parent 031b1ad commit ef74d54
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Added equals/hashcode for named DocValueFormat.DateTime inner class ([#6357](https://github.com/opensearch-project/OpenSearch/pull/6357))
- Fixed bug for searchable snapshot to take 'base_path' of blob into account ([#6558](https://github.com/opensearch-project/OpenSearch/pull/6558))
- Fix fuzziness validation ([#5805](https://github.com/opensearch-project/OpenSearch/pull/5805))
- Avoid negative memory result in IndicesQueryCache stats calculation ([#6917](https://github.com/opensearch-project/OpenSearch/pull/6917))
- Fix GetSnapshots to not return non-existent snapshots with ignore_unavailable=true ([#6839](https://github.com/opensearch-project/OpenSearch/pull/6839))

### Security
Expand Down
16 changes: 10 additions & 6 deletions server/src/main/java/org/opensearch/indices/IndicesQueryCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,17 @@ public QueryCacheStats getStats(ShardId shard) {

// We also have some shared ram usage that we try to distribute to
// proportionally to their number of cache entries of each shard
long totalSize = 0;
for (QueryCacheStats s : stats.values()) {
totalSize += s.getCacheSize();
if (stats.isEmpty()) {
shardStats.add(new QueryCacheStats(sharedRamBytesUsed, 0, 0, 0, 0));
} else {
long totalSize = 0;
for (QueryCacheStats s : stats.values()) {
totalSize += s.getCacheSize();
}
final double weight = totalSize == 0 ? 1d / stats.size() : ((double) shardStats.getCacheSize()) / totalSize;
final long additionalRamBytesUsed = Math.round(weight * sharedRamBytesUsed);
shardStats.add(new QueryCacheStats(additionalRamBytesUsed, 0, 0, 0, 0));
}
final double weight = totalSize == 0 ? 1d / stats.size() : ((double) shardStats.getCacheSize()) / totalSize;
final long additionalRamBytesUsed = Math.round(weight * sharedRamBytesUsed);
shardStats.add(new QueryCacheStats(additionalRamBytesUsed, 0, 0, 0, 0));
return shardStats;
}

Expand Down
18 changes: 18 additions & 0 deletions server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ public void testBasics() throws IOException {
assertEquals(0L, stats.getCacheCount());
assertEquals(0L, stats.getHitCount());
assertEquals(0L, stats.getMissCount());
assertEquals(0L, stats.getMemorySizeInBytes());

assertEquals(1, s.count(new DummyQuery(0)));

Expand All @@ -151,6 +152,7 @@ public void testBasics() throws IOException {
assertEquals(1L, stats.getCacheCount());
assertEquals(0L, stats.getHitCount());
assertEquals(1L, stats.getMissCount());
assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE);

for (int i = 1; i < 20; ++i) {
assertEquals(1, s.count(new DummyQuery(i)));
Expand All @@ -161,6 +163,7 @@ public void testBasics() throws IOException {
assertEquals(20L, stats.getCacheCount());
assertEquals(0L, stats.getHitCount());
assertEquals(20L, stats.getMissCount());
assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE);

s.count(new DummyQuery(10));

Expand All @@ -169,6 +172,7 @@ public void testBasics() throws IOException {
assertEquals(20L, stats.getCacheCount());
assertEquals(1L, stats.getHitCount());
assertEquals(20L, stats.getMissCount());
assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE);

IOUtils.close(r, dir);

Expand All @@ -178,6 +182,7 @@ public void testBasics() throws IOException {
assertEquals(20L, stats.getCacheCount());
assertEquals(1L, stats.getHitCount());
assertEquals(20L, stats.getMissCount());
assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE);

cache.onClose(shard);

Expand All @@ -187,6 +192,7 @@ public void testBasics() throws IOException {
assertEquals(0L, stats.getCacheCount());
assertEquals(0L, stats.getHitCount());
assertEquals(0L, stats.getMissCount());
assertTrue(stats.getMemorySizeInBytes() >= 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE);

cache.close(); // this triggers some assertions
}
Expand Down Expand Up @@ -227,12 +233,14 @@ public void testTwoShards() throws IOException {
assertEquals(1L, stats1.getCacheCount());
assertEquals(0L, stats1.getHitCount());
assertEquals(1L, stats1.getMissCount());
assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE);

QueryCacheStats stats2 = cache.getStats(shard2);
assertEquals(0L, stats2.getCacheSize());
assertEquals(0L, stats2.getCacheCount());
assertEquals(0L, stats2.getHitCount());
assertEquals(0L, stats2.getMissCount());
assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE);

assertEquals(1, s2.count(new DummyQuery(0)));

Expand All @@ -241,12 +249,14 @@ public void testTwoShards() throws IOException {
assertEquals(1L, stats1.getCacheCount());
assertEquals(0L, stats1.getHitCount());
assertEquals(1L, stats1.getMissCount());
assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE);

stats2 = cache.getStats(shard2);
assertEquals(1L, stats2.getCacheSize());
assertEquals(1L, stats2.getCacheCount());
assertEquals(0L, stats2.getHitCount());
assertEquals(1L, stats2.getMissCount());
assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE);

for (int i = 0; i < 20; ++i) {
assertEquals(1, s2.count(new DummyQuery(i)));
Expand All @@ -257,12 +267,14 @@ public void testTwoShards() throws IOException {
assertEquals(1L, stats1.getCacheCount());
assertEquals(0L, stats1.getHitCount());
assertEquals(1L, stats1.getMissCount());
assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE);

stats2 = cache.getStats(shard2);
assertEquals(10L, stats2.getCacheSize());
assertEquals(20L, stats2.getCacheCount());
assertEquals(1L, stats2.getHitCount());
assertEquals(20L, stats2.getMissCount());
assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE);

IOUtils.close(r1, dir1);

Expand All @@ -272,12 +284,14 @@ public void testTwoShards() throws IOException {
assertEquals(1L, stats1.getCacheCount());
assertEquals(0L, stats1.getHitCount());
assertEquals(1L, stats1.getMissCount());
assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE);

stats2 = cache.getStats(shard2);
assertEquals(10L, stats2.getCacheSize());
assertEquals(20L, stats2.getCacheCount());
assertEquals(1L, stats2.getHitCount());
assertEquals(20L, stats2.getMissCount());
assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE);

cache.onClose(shard1);

Expand All @@ -287,12 +301,14 @@ public void testTwoShards() throws IOException {
assertEquals(0L, stats1.getCacheCount());
assertEquals(0L, stats1.getHitCount());
assertEquals(0L, stats1.getMissCount());
assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE);

stats2 = cache.getStats(shard2);
assertEquals(10L, stats2.getCacheSize());
assertEquals(20L, stats2.getCacheCount());
assertEquals(1L, stats2.getHitCount());
assertEquals(20L, stats2.getMissCount());
assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE);

IOUtils.close(r2, dir2);
cache.onClose(shard2);
Expand All @@ -303,12 +319,14 @@ public void testTwoShards() throws IOException {
assertEquals(0L, stats1.getCacheCount());
assertEquals(0L, stats1.getHitCount());
assertEquals(0L, stats1.getMissCount());
assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE);

stats2 = cache.getStats(shard2);
assertEquals(0L, stats2.getCacheSize());
assertEquals(0L, stats2.getCacheCount());
assertEquals(0L, stats2.getHitCount());
assertEquals(0L, stats2.getMissCount());
assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE);

cache.close(); // this triggers some assertions
}
Expand Down

0 comments on commit ef74d54

Please sign in to comment.