Skip to content

Commit

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

(#6917)

* Avoid negative memory result in IndicesQueryCache stats calculation

Signed-off-by: Daniel Widdis <[email protected]>

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

Signed-off-by: Daniel Widdis <[email protected]>

* Add test which actually fails without the bug fix

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Daniel Widdis <[email protected]>
(cherry picked from commit c0a8cf8)
  • Loading branch information
@dbwiddis
dbwiddis authored Apr 12, 2023
1 parent 74a695b commit 5b4ed15
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 6 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### Deprecated
### Removed
### Fixed
- Avoid negative memory result in IndicesQueryCache stats calculation ([#6917](https://github.com/opensearch-project/OpenSearch/pull/6917))

### Security

[Unreleased 1.3.x]: https://github.com/opensearch-project/OpenSearch/compare/1.3.9...HEAD
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 @@ -124,13 +124,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 @@ -139,6 +139,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 @@ -147,6 +148,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 @@ -157,6 +159,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 @@ -165,6 +168,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 @@ -174,6 +178,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 @@ -183,6 +188,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 @@ -223,12 +229,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 @@ -237,12 +245,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 @@ -253,12 +263,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 @@ -268,12 +280,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 @@ -283,12 +297,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 @@ -299,12 +315,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 5b4ed15

Please sign in to comment.