From 5b4ed153e77b1ab56ac072ebb3ce46427b87d79e Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Wed, 12 Apr 2023 10:12:29 -0700 Subject: [PATCH] Avoid negative memory result in IndicesQueryCache stats calculation (#6966) (#6917) * Avoid negative memory result in IndicesQueryCache stats calculation Signed-off-by: Daniel Widdis * Cache stores recent queries so isn't empty even with no doc id sets Signed-off-by: Daniel Widdis * Add test which actually fails without the bug fix Signed-off-by: Daniel Widdis --------- Signed-off-by: Daniel Widdis (cherry picked from commit c0a8cf8823341cf92b932fe3d12909d427090a2f) --- CHANGELOG.md | 2 ++ .../opensearch/indices/IndicesQueryCache.java | 16 ++++++++++------ .../indices/IndicesQueryCacheTests.java | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad645fd5162cd..693d288872832 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java b/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java index 9c7d66457ce15..e764d15b4325c 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java @@ -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; } diff --git a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java index 383c0277e1c27..8aa9b36774f33 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java @@ -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))); @@ -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))); @@ -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)); @@ -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); @@ -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); @@ -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 } @@ -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))); @@ -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))); @@ -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); @@ -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); @@ -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); @@ -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 }