From 8fe15e6110986b05b65afe6c811deb4e347c985c Mon Sep 17 00:00:00 2001 From: Austin Lee Date: Thu, 15 Feb 2024 15:51:39 -0800 Subject: [PATCH] =?UTF-8?q?Fix=20a=20flaky=20test=20that=20relies=20on=20t?= =?UTF-8?q?erminate=5Fafter=20for=20an=20exact=20count=20ma=E2=80=A6=20(#1?= =?UTF-8?q?2179)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix a flaky test that relies on terminate_after for an exact count match. (Issue # 10435) Signed-off-by: Austin Lee * Make comments HTML Javadoc friendly. Signed-off-by: Austin Lee * Fix javadoc syntax Signed-off-by: Andrew Ross --------- Signed-off-by: Austin Lee Signed-off-by: Andrew Ross Co-authored-by: Andrew Ross --- .../search/simple/SimpleSearchIT.java | 81 +++++++++++++------ .../test/hamcrest/OpenSearchAssertions.java | 16 ++++ 2 files changed, 73 insertions(+), 24 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/search/simple/SimpleSearchIT.java b/server/src/internalClusterTest/java/org/opensearch/search/simple/SimpleSearchIT.java index 0606bc1dbbdb6..1c1587a3be600 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/simple/SimpleSearchIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/simple/SimpleSearchIT.java @@ -299,7 +299,15 @@ public void dotestSimpleTerminateAfterCountWithSize(int size, int max) throws Ex .setSize(size) .setTrackTotalHits(true) .get(); - assertHitCount(searchResponse, i); + + // Do not expect an exact match as an optimization introduced by https://issues.apache.org/jira/browse/LUCENE-10620 + // can produce a total hit count > terminated_after, but this only kicks in + // when size = 0 which is when TotalHitCountCollector is used. + if (size == 0) { + assertHitCount(searchResponse, i, max); + } else { + assertHitCount(searchResponse, i); + } assertTrue(searchResponse.isTerminatedEarly()); assertEquals(Math.min(i, size), searchResponse.getHits().getHits().length); } @@ -313,7 +321,6 @@ public void dotestSimpleTerminateAfterCountWithSize(int size, int max) throws Ex assertFalse(searchResponse.isTerminatedEarly()); } - @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/10435") public void testSimpleTerminateAfterCountSize0() throws Exception { int max = randomIntBetween(3, 29); dotestSimpleTerminateAfterCountWithSize(0, max); @@ -324,6 +331,24 @@ public void testSimpleTerminateAfterCountRandomSize() throws Exception { dotestSimpleTerminateAfterCountWithSize(randomIntBetween(1, max), max); } + /** + * Special cases when size = 0: + * + * If track_total_hits = true: + * Weight#count optimization can cause totalHits in the response to be up to the total doc count regardless of terminate_after. + * So, we will have to do a range check, not an equality check. + * + * If track_total_hits != true, but set to a value AND terminate_after is set: + * Again, due to the optimization, any count can be returned. + * Up to terminate_after, relation == EQUAL_TO. + * But if track_total_hits_up_to ≥ terminate_after, relation can be EQ _or_ GTE. + * This ambiguity is due to the fact that totalHits == track_total_hits_up_to + * or totalHits > track_total_hits_up_to and SearchPhaseController sets totalHits = track_total_hits_up_to when returning results + * in which case relation = GTE. + * + * @param size + * @throws Exception + */ public void doTestSimpleTerminateAfterTrackTotalHitsUpTo(int size) throws Exception { prepareCreate("test").setSettings(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 1).put(SETTING_NUMBER_OF_REPLICAS, 0)).get(); ensureGreen(); @@ -340,6 +365,7 @@ public void doTestSimpleTerminateAfterTrackTotalHitsUpTo(int size) throws Except refresh(); SearchResponse searchResponse; + searchResponse = client().prepareSearch("test") .setQuery(QueryBuilders.rangeQuery("field").gte(1).lte(numDocs)) .setTerminateAfter(10) @@ -350,25 +376,28 @@ public void doTestSimpleTerminateAfterTrackTotalHitsUpTo(int size) throws Except assertEquals(5, searchResponse.getHits().getTotalHits().value); assertEquals(GREATER_THAN_OR_EQUAL_TO, searchResponse.getHits().getTotalHits().relation); - searchResponse = client().prepareSearch("test") - .setQuery(QueryBuilders.rangeQuery("field").gte(1).lte(numDocs)) - .setTerminateAfter(5) - .setSize(size) - .setTrackTotalHitsUpTo(10) - .get(); - assertTrue(searchResponse.isTerminatedEarly()); - assertEquals(5, searchResponse.getHits().getTotalHits().value); - assertEquals(EQUAL_TO, searchResponse.getHits().getTotalHits().relation); + // For size = 0, the following queries terminate early, but hits and relation can vary. + if (size > 0) { + searchResponse = client().prepareSearch("test") + .setQuery(QueryBuilders.rangeQuery("field").gte(1).lte(numDocs)) + .setTerminateAfter(5) + .setSize(size) + .setTrackTotalHitsUpTo(10) + .get(); + assertTrue(searchResponse.isTerminatedEarly()); + assertEquals(5, searchResponse.getHits().getTotalHits().value); + assertEquals(EQUAL_TO, searchResponse.getHits().getTotalHits().relation); - searchResponse = client().prepareSearch("test") - .setQuery(QueryBuilders.rangeQuery("field").gte(1).lte(numDocs)) - .setTerminateAfter(5) - .setSize(size) - .setTrackTotalHitsUpTo(5) - .get(); - assertTrue(searchResponse.isTerminatedEarly()); - assertEquals(5, searchResponse.getHits().getTotalHits().value); - assertEquals(EQUAL_TO, searchResponse.getHits().getTotalHits().relation); + searchResponse = client().prepareSearch("test") + .setQuery(QueryBuilders.rangeQuery("field").gte(1).lte(numDocs)) + .setTerminateAfter(5) + .setSize(size) + .setTrackTotalHitsUpTo(5) + .get(); + assertTrue(searchResponse.isTerminatedEarly()); + assertEquals(5, searchResponse.getHits().getTotalHits().value); + assertEquals(EQUAL_TO, searchResponse.getHits().getTotalHits().relation); + } searchResponse = client().prepareSearch("test") .setQuery(QueryBuilders.rangeQuery("field").gte(1).lte(numDocs)) @@ -377,7 +406,12 @@ public void doTestSimpleTerminateAfterTrackTotalHitsUpTo(int size) throws Except .setTrackTotalHits(true) .get(); assertTrue(searchResponse.isTerminatedEarly()); - assertEquals(5, searchResponse.getHits().getTotalHits().value); + if (size == 0) { + // Since terminate_after < track_total_hits, we need to do a range check. + assertHitCount(searchResponse, 5, numDocs); + } else { + assertEquals(5, searchResponse.getHits().getTotalHits().value); + } assertEquals(EQUAL_TO, searchResponse.getHits().getTotalHits().relation); searchResponse = client().prepareSearch("test") @@ -399,12 +433,11 @@ public void doTestSimpleTerminateAfterTrackTotalHitsUpTo(int size) throws Except assertEquals(GREATER_THAN_OR_EQUAL_TO, searchResponse.getHits().getTotalHits().relation); } - @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/10435") - public void testSimpleTerminateAfterTrackTotalHitsUpToRandomSize() throws Exception { + public void testSimpleTerminateAfterTrackTotalHitsUpToRandomSize0() throws Exception { doTestSimpleTerminateAfterTrackTotalHitsUpTo(0); } - public void testSimpleTerminateAfterTrackTotalHitsUpToSize0() throws Exception { + public void testSimpleTerminateAfterTrackTotalHitsUpToSize() throws Exception { doTestSimpleTerminateAfterTrackTotalHitsUpTo(randomIntBetween(1, 29)); } diff --git a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java index 9b0de13c35ec8..650558aaa97a6 100644 --- a/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java +++ b/test/framework/src/main/java/org/opensearch/test/hamcrest/OpenSearchAssertions.java @@ -304,6 +304,22 @@ public static void assertHitCount(SearchResponse countResponse, long expectedHit } } + public static void assertHitCount(SearchResponse countResponse, long minHitCount, long maxHitCount) { + final TotalHits totalHits = countResponse.getHits().getTotalHits(); + if (!(totalHits.relation == TotalHits.Relation.EQUAL_TO && totalHits.value >= minHitCount && totalHits.value <= maxHitCount)) { + fail( + "Count is " + + totalHits + + " not between " + + minHitCount + + " and " + + maxHitCount + + " inclusive. " + + formatShardStatus(countResponse) + ); + } + } + public static void assertExists(GetResponse response) { String message = String.format(Locale.ROOT, "Expected %s/%s to exist, but does not", response.getIndex(), response.getId()); assertThat(message, response.isExists(), is(true));