From ccb6566c6b987bcd1f4b982bc6fdf0d6342f5ec8 Mon Sep 17 00:00:00 2001 From: Niyati Aggarwal Date: Tue, 10 Oct 2023 13:04:47 -0700 Subject: [PATCH 1/6] adding delegation from CachingWeightWrapper#count to internal weight object and its unit test Signed-off-by: Niyati Aggarwal --- .../main/java/org/opensearch/indices/IndicesQueryCache.java | 5 +++++ .../java/org/opensearch/indices/IndicesQueryCacheTests.java | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java b/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java index 228723045d8cc..b25ab7b50bff9 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java @@ -186,6 +186,11 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException { return in.bulkScorer(context); } + @Override + public int count(LeafReaderContext context) throws IOException { + return in.count(context); + } + @Override public boolean isCacheable(LeafReaderContext ctx) { return in.isCacheable(ctx); diff --git a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java index 58113f0123c8e..5912440e9aea0 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java @@ -388,6 +388,7 @@ public void testStatsOnEviction() throws IOException { private static class DummyWeight extends Weight { private final Weight weight; + private final int randCount = randomIntBetween(0, Integer.MAX_VALUE); private boolean scorerCalled; private boolean scorerSupplierCalled; @@ -413,6 +414,9 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti return weight.scorerSupplier(context); } + @Override + public int count(LeafReaderContext context) throws IOException { return randCount; } + @Override public boolean isCacheable(LeafReaderContext ctx) { return true; @@ -454,6 +458,8 @@ public void onUse(Query query) {} cached.scorerSupplier(s.getIndexReader().leaves().get(0)); assertFalse(weight.scorerCalled); assertTrue(weight.scorerSupplierCalled); + // verifying CachingWeightWrapper#count delegates to DummyWeight#count + assertEquals(weight.count(null), cached.count(null)); IOUtils.close(r, dir); cache.onClose(shard); cache.close(); From ca544556ea3641288a295b15efe6fc0306afa93d Mon Sep 17 00:00:00 2001 From: Niyati Aggarwal Date: Tue, 10 Oct 2023 18:20:50 -0700 Subject: [PATCH 2/6] fixing checkstyle violations Signed-off-by: Niyati Aggarwal --- .../java/org/opensearch/indices/IndicesQueryCacheTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java index 5912440e9aea0..ca129cc35f996 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java @@ -415,7 +415,9 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti } @Override - public int count(LeafReaderContext context) throws IOException { return randCount; } + public int count(LeafReaderContext context) throws IOException { + return randCount; + } @Override public boolean isCacheable(LeafReaderContext ctx) { From 84cc0b243fbde57068aace3a4b576c1b51f3f6a0 Mon Sep 17 00:00:00 2001 From: Niyati Aggarwal Date: Wed, 11 Oct 2023 14:23:20 -0700 Subject: [PATCH 3/6] fixing existing tests Signed-off-by: Niyati Aggarwal --- .../opensearch/indices/IndicesQueryCache.java | 1 + .../indices/IndicesQueryCacheTests.java | 26 +++++++++---------- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java b/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java index b25ab7b50bff9..136bfd48f8759 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java +++ b/server/src/main/java/org/opensearch/indices/IndicesQueryCache.java @@ -188,6 +188,7 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException { @Override public int count(LeafReaderContext context) throws IOException { + shardKeyMap.add(context.reader()); return in.count(context); } diff --git a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java index ca129cc35f996..a59caad16fc6d 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java @@ -151,7 +151,7 @@ public void testBasics() throws IOException { assertEquals(1L, stats.getCacheSize()); assertEquals(1L, stats.getCacheCount()); assertEquals(0L, stats.getHitCount()); - assertEquals(1L, stats.getMissCount()); + assertEquals(2L, stats.getMissCount()); assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE); for (int i = 1; i < 20; ++i) { @@ -162,7 +162,7 @@ public void testBasics() throws IOException { assertEquals(10L, stats.getCacheSize()); assertEquals(20L, stats.getCacheCount()); assertEquals(0L, stats.getHitCount()); - assertEquals(20L, stats.getMissCount()); + assertEquals(40L, stats.getMissCount()); assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE); s.count(new DummyQuery(10)); @@ -171,7 +171,7 @@ public void testBasics() throws IOException { assertEquals(10L, stats.getCacheSize()); assertEquals(20L, stats.getCacheCount()); assertEquals(1L, stats.getHitCount()); - assertEquals(20L, stats.getMissCount()); + assertEquals(40L, stats.getMissCount()); assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE); IOUtils.close(r, dir); @@ -181,7 +181,7 @@ public void testBasics() throws IOException { assertEquals(0L, stats.getCacheSize()); assertEquals(20L, stats.getCacheCount()); assertEquals(1L, stats.getHitCount()); - assertEquals(20L, stats.getMissCount()); + assertEquals(40L, stats.getMissCount()); assertTrue(stats.getMemorySizeInBytes() > 0L && stats.getMemorySizeInBytes() < Long.MAX_VALUE); cache.onClose(shard); @@ -232,7 +232,7 @@ public void testTwoShards() throws IOException { assertEquals(1L, stats1.getCacheSize()); assertEquals(1L, stats1.getCacheCount()); assertEquals(0L, stats1.getHitCount()); - assertEquals(1L, stats1.getMissCount()); + assertEquals(2L, stats1.getMissCount()); assertTrue(stats1.getMemorySizeInBytes() >= 0L && stats1.getMemorySizeInBytes() < Long.MAX_VALUE); QueryCacheStats stats2 = cache.getStats(shard2); @@ -248,14 +248,14 @@ public void testTwoShards() throws IOException { assertEquals(1L, stats1.getCacheSize()); assertEquals(1L, stats1.getCacheCount()); assertEquals(0L, stats1.getHitCount()); - assertEquals(1L, stats1.getMissCount()); + assertEquals(2L, 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()); + assertEquals(2L, stats2.getMissCount()); assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE); for (int i = 0; i < 20; ++i) { @@ -266,14 +266,14 @@ public void testTwoShards() throws IOException { assertEquals(0L, stats1.getCacheSize()); // evicted assertEquals(1L, stats1.getCacheCount()); assertEquals(0L, stats1.getHitCount()); - assertEquals(1L, stats1.getMissCount()); + assertEquals(2L, 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()); + assertEquals(40L, stats2.getMissCount()); assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE); IOUtils.close(r1, dir1); @@ -283,14 +283,14 @@ public void testTwoShards() throws IOException { assertEquals(0L, stats1.getCacheSize()); assertEquals(1L, stats1.getCacheCount()); assertEquals(0L, stats1.getHitCount()); - assertEquals(1L, stats1.getMissCount()); + assertEquals(2L, 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()); + assertEquals(40L, stats2.getMissCount()); assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE); cache.onClose(shard1); @@ -307,7 +307,7 @@ public void testTwoShards() throws IOException { assertEquals(10L, stats2.getCacheSize()); assertEquals(20L, stats2.getCacheCount()); assertEquals(1L, stats2.getHitCount()); - assertEquals(20L, stats2.getMissCount()); + assertEquals(40L, stats2.getMissCount()); assertTrue(stats2.getMemorySizeInBytes() >= 0L && stats2.getMemorySizeInBytes() < Long.MAX_VALUE); IOUtils.close(r2, dir2); @@ -460,8 +460,6 @@ public void onUse(Query query) {} cached.scorerSupplier(s.getIndexReader().leaves().get(0)); assertFalse(weight.scorerCalled); assertTrue(weight.scorerSupplierCalled); - // verifying CachingWeightWrapper#count delegates to DummyWeight#count - assertEquals(weight.count(null), cached.count(null)); IOUtils.close(r, dir); cache.onClose(shard); cache.close(); From d1842d878bd34fd0abefb5cb772c29dfabe31ebd Mon Sep 17 00:00:00 2001 From: Niyati Aggarwal Date: Fri, 13 Oct 2023 12:03:48 -0700 Subject: [PATCH 4/6] adding test for delegating count to internal weight object Signed-off-by: Niyati Aggarwal --- .../indices/IndicesQueryCacheTests.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java index a59caad16fc6d..ba40343fb2130 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesQueryCacheTests.java @@ -391,6 +391,7 @@ private static class DummyWeight extends Weight { private final int randCount = randomIntBetween(0, Integer.MAX_VALUE); private boolean scorerCalled; private boolean scorerSupplierCalled; + private boolean countCalled; DummyWeight(Weight weight) { super(weight.getQuery()); @@ -416,6 +417,7 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti @Override public int count(LeafReaderContext context) throws IOException { + countCalled = true; return randCount; } @@ -464,4 +466,26 @@ public void onUse(Query query) {} cache.onClose(shard); cache.close(); } + + public void testDelegatesCount() throws Exception { + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig()); + w.addDocument(new Document()); + DirectoryReader r = DirectoryReader.open(w); + w.close(); + ShardId shard = new ShardId("index", "_na_", 0); + r = OpenSearchDirectoryReader.wrap(r, shard); + IndexSearcher s = new IndexSearcher(r); + IndicesQueryCache cache = new IndicesQueryCache(Settings.EMPTY); + s.setQueryCache(cache); + Query query = new MatchAllDocsQuery(); + final DummyWeight weight = new DummyWeight(s.createWeight(s.rewrite(query), ScoreMode.COMPLETE_NO_SCORES, 1f)); + final Weight cached = cache.doCache(weight, s.getQueryCachingPolicy()); + assertFalse(weight.countCalled); + assertEquals(weight.randCount, cached.count(s.getIndexReader().leaves().get(0))); + assertTrue(weight.countCalled); + IOUtils.close(r, dir); + cache.onClose(shard); + cache.close(); + } } From 19493a0a140b7f4021088d9b9bb40617f7eae8cf Mon Sep 17 00:00:00 2001 From: Niyati Aggarwal Date: Tue, 21 Nov 2023 14:21:28 +0530 Subject: [PATCH 5/6] Fixing failing unit tests Signed-off-by: Niyati Aggarwal --- .../indices/IndicesServiceCloseTests.java | 65 ++++++++++++++++++- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java index 415844dccb611..add1dfc348a8b 100644 --- a/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/opensearch/indices/IndicesServiceCloseTests.java @@ -32,8 +32,16 @@ package org.opensearch.indices; -import org.apache.lucene.document.LongPoint; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.ConstantScoreScorer; +import org.apache.lucene.search.ConstantScoreWeight; +import org.apache.lucene.search.DocIdSetIterator; +import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.Query; +import org.apache.lucene.search.QueryVisitor; +import org.apache.lucene.search.ScoreMode; +import org.apache.lucene.search.Scorer; +import org.apache.lucene.search.Weight; import org.opensearch.cluster.ClusterName; import org.opensearch.cluster.routing.allocation.DiskThresholdSettings; import org.opensearch.common.cache.RemovalNotification; @@ -59,6 +67,7 @@ import org.opensearch.test.hamcrest.OpenSearchAssertions; import org.opensearch.transport.nio.MockNioTransportPlugin; +import java.io.IOException; import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; @@ -73,6 +82,56 @@ public class IndicesServiceCloseTests extends OpenSearchTestCase { + private static class DummyQuery extends Query { + + private final int id; + + DummyQuery(int id) { + this.id = id; + } + + @Override + public void visit(QueryVisitor visitor) { + visitor.visitLeaf(this); + } + + @Override + public boolean equals(Object obj) { + return sameClassAs(obj) && id == ((IndicesServiceCloseTests.DummyQuery) obj).id; + } + + @Override + public int hashCode() { + return 31 * classHash() + id; + } + + @Override + public String toString(String field) { + return "dummy"; + } + + @Override + public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException { + return new ConstantScoreWeight(this, boost) { + @Override + public Scorer scorer(LeafReaderContext context) throws IOException { + return new ConstantScoreScorer(this, score(), scoreMode, DocIdSetIterator.all(context.reader().maxDoc())); + } + + @Override + public int count(LeafReaderContext context) { + return -1; + } + + @Override + public boolean isCacheable(LeafReaderContext ctx) { + return true; + } + }; + } + + } + private Node startNode() throws NodeValidationException { final Path tempDir = createTempDir(); String nodeName = "node_s_0"; @@ -225,7 +284,7 @@ public void testCloseAfterRequestHasUsedQueryCache() throws Exception { Engine.Searcher searcher = shard.acquireSearcher("test"); assertEquals(1, searcher.getIndexReader().maxDoc()); - Query query = LongPoint.newRangeQuery("foo", 0, 5); + Query query = new DummyQuery(1); assertEquals(0L, cache.getStats(shard.shardId()).getCacheSize()); searcher.count(query); assertEquals(1L, cache.getStats(shard.shardId()).getCacheSize()); @@ -271,7 +330,7 @@ public void testCloseWhileOngoingRequestUsesQueryCache() throws Exception { node.close(); assertEquals(1, indicesService.indicesRefCount.refCount()); - Query query = LongPoint.newRangeQuery("foo", 0, 5); + Query query = new DummyQuery(1); assertEquals(0L, cache.getStats(shard.shardId()).getCacheSize()); searcher.count(query); assertEquals(1L, cache.getStats(shard.shardId()).getCacheSize()); From 1affddff2bdf5296353a08b820ed02846cd6ef1f Mon Sep 17 00:00:00 2001 From: Niyati Aggarwal Date: Wed, 22 Nov 2023 09:03:50 +0530 Subject: [PATCH 6/6] Adding the change log Signed-off-by: Niyati Aggarwal --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ba21c5278aca..6d09e7dd0d702 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix passing wrong parameter when calling newConfigurationException() in DotExpanderProcessor ([#10737](https://github.com/opensearch-project/OpenSearch/pull/10737)) - Fix SuggestSearch.testSkipDuplicates by forceing refresh when indexing its test documents ([#11068](https://github.com/opensearch-project/OpenSearch/pull/11068)) - Adding version condition while adding geoshape doc values to the index, to ensure backward compatibility.([#11095](https://github.com/opensearch-project/OpenSearch/pull/11095)) +- Delegating CachingWeightWrapper#count to internal weight object ([#10543](https://github.com/opensearch-project/OpenSearch/pull/10543)) - Fix per request latency last phase not tracked ([#10934](https://github.com/opensearch-project/OpenSearch/pull/10934)) ### Security