From 493104f5f74adc6f57833b525fa7e6f5f9edeffc Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 21 May 2024 18:47:18 -0700 Subject: [PATCH 01/12] Added timeout for QA distributed query for shard.tolerant --- .../solr/handler/component/HttpShardHandler.java | 16 ++++++++++++---- .../solr/handler/component/SearchHandler.java | 2 +- .../solr/handler/component/ShardHandler.java | 2 ++ .../solr/core/MockShardHandlerFactory.java | 5 +++++ 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java index 0ad76294335..a871623416e 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java @@ -223,7 +223,12 @@ protected ShardResponse transfomResponse( */ @Override public ShardResponse takeCompletedIncludingErrors() { - return take(false); + return take(false, Long.MAX_VALUE); + } + + @Override + public ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTime) { + return take(false, maxAllowedTime); } /** @@ -232,13 +237,16 @@ public ShardResponse takeCompletedIncludingErrors() { */ @Override public ShardResponse takeCompletedOrError() { - return take(true); + return take(true, Long.MAX_VALUE); } - private ShardResponse take(boolean bailOnError) { + private ShardResponse take(boolean bailOnError, long maxAllowedTime) { try { while (pending.get() > 0) { - ShardResponse rsp = responses.take(); + long waitTime = maxAllowedTime - System.nanoTime(); + ShardResponse rsp = responses.poll(waitTime, TimeUnit.NANOSECONDS); + if(rsp ==null) + return null; responseCancellableMap.remove(rsp); pending.decrementAndGet(); diff --git a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java index 7470852eaa2..60cb42d25d6 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java @@ -529,7 +529,7 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw } } else { // a distributed request - + long maxTimeAllowed = req.getParams().getLong(CommonParams.TIME_ALLOWED, Long.MAX_VALUE); if (rb.outgoing == null) { rb.outgoing = new ArrayList<>(); } diff --git a/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java index d56ad24d7c5..41fa3f45423 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java @@ -25,6 +25,8 @@ public abstract class ShardHandler { public abstract ShardResponse takeCompletedIncludingErrors(); + public abstract ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTime); + public abstract ShardResponse takeCompletedOrError(); public abstract void cancelAll(); diff --git a/solr/core/src/test/org/apache/solr/core/MockShardHandlerFactory.java b/solr/core/src/test/org/apache/solr/core/MockShardHandlerFactory.java index 299f751393e..c86afdb4ec3 100644 --- a/solr/core/src/test/org/apache/solr/core/MockShardHandlerFactory.java +++ b/solr/core/src/test/org/apache/solr/core/MockShardHandlerFactory.java @@ -48,6 +48,11 @@ public ShardResponse takeCompletedIncludingErrors() { return null; } + @Override + public ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTime) { + return null; + } + @Override public ShardResponse takeCompletedOrError() { return null; From 44582f2e98a955dca228b1893641208830925b9d Mon Sep 17 00:00:00 2001 From: hitesh Date: Sat, 25 May 2024 10:53:51 -0700 Subject: [PATCH 02/12] Added test and updated Michael Feedback --- .../handler/component/HttpShardHandler.java | 32 ++++++++--- .../solr/handler/component/SearchHandler.java | 5 +- .../solr/handler/component/ShardHandler.java | 3 +- .../TestHttpShardHandlerFactory.java | 54 +++++++++++++++++++ 4 files changed, 83 insertions(+), 11 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java index a871623416e..f9501c739ee 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java @@ -223,12 +223,12 @@ protected ShardResponse transfomResponse( */ @Override public ShardResponse takeCompletedIncludingErrors() { - return take(false, Long.MAX_VALUE); + return take(false, -1); } @Override - public ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTime) { - return take(false, maxAllowedTime); + public ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTimeInMillis) { + return take(false, maxAllowedTimeInMillis); } /** @@ -237,16 +237,22 @@ public ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTime */ @Override public ShardResponse takeCompletedOrError() { - return take(true, Long.MAX_VALUE); + return take(true, -1); } - private ShardResponse take(boolean bailOnError, long maxAllowedTime) { + private ShardResponse take(boolean bailOnError, long maxAllowedTimeInMillis) { try { + long deadline = System.nanoTime(); + if (maxAllowedTimeInMillis > 0) { + deadline += TimeUnit.MILLISECONDS.toNanos(maxAllowedTimeInMillis); + } else { + deadline = Long.MAX_VALUE; + } + while (pending.get() > 0) { - long waitTime = maxAllowedTime - System.nanoTime(); + long waitTime = deadline - System.nanoTime(); ShardResponse rsp = responses.poll(waitTime, TimeUnit.NANOSECONDS); - if(rsp ==null) - return null; + if (rsp == null) return null; responseCancellableMap.remove(rsp); pending.decrementAndGet(); @@ -411,4 +417,14 @@ private boolean canShortCircuit( public ShardHandlerFactory getShardHandlerFactory() { return httpShardHandlerFactory; } + + // test helper function + void setPendingRequest(int val) { + this.pending.set(val); + } + + // test helper function + void setResponse(ShardResponse shardResponse) { + this.responses.add(shardResponse); + } } diff --git a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java index 60cb42d25d6..c2aeeb231b2 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java @@ -529,7 +529,8 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw } } else { // a distributed request - long maxTimeAllowed = req.getParams().getLong(CommonParams.TIME_ALLOWED, Long.MAX_VALUE); + long maxTimeAllowed = req.getParams().getLong(CommonParams.TIME_ALLOWED, -1); + if (rb.outgoing == null) { rb.outgoing = new ArrayList<>(); } @@ -593,7 +594,7 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw while (rb.outgoing.size() == 0) { ShardResponse srsp = tolerant - ? shardHandler1.takeCompletedIncludingErrors() + ? shardHandler1.takeCompletedIncludingErrorsWithTimeout(maxTimeAllowed) : shardHandler1.takeCompletedOrError(); if (srsp == null) break; // no more requests to wait for diff --git a/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java index 41fa3f45423..ebeae4ccf22 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/ShardHandler.java @@ -25,7 +25,8 @@ public abstract class ShardHandler { public abstract ShardResponse takeCompletedIncludingErrors(); - public abstract ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTime); + public abstract ShardResponse takeCompletedIncludingErrorsWithTimeout( + long maxAllowedTimeInMillis); public abstract ShardResponse takeCompletedOrError(); diff --git a/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java b/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java index 6c20bdcdf08..737803186b0 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java +++ b/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java @@ -27,6 +27,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.TimeUnit; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.client.solrj.impl.LBSolrClient; import org.apache.solr.client.solrj.request.QueryRequest; @@ -155,4 +156,57 @@ public void testLiveNodesToHostUrl() { MatcherAssert.assertThat(hostSet, hasItem("1.2.3.4:9000")); MatcherAssert.assertThat(hostSet, hasItem("1.2.3.4:9001")); } + + @Test + public void testHttpShardHandlerTimeout() { + HttpShardHandlerFactory httpShardHandlerFactory = new HttpShardHandlerFactory(); + HttpShardHandler shardHandler = (HttpShardHandler) httpShardHandlerFactory.getShardHandler(); + + long startTime = System.nanoTime(); + long timeAllowedInMillis = 120; + shardHandler.setPendingRequest(1); + ShardResponse shardResponse = + shardHandler.takeCompletedIncludingErrorsWithTimeout(timeAllowedInMillis); + + assertEquals(null, shardResponse); + + long timeTakenInMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime); + assertTrue( + "Should have taken more than 100 milli seconds " + timeTakenInMillis, + timeTakenInMillis > 100); + assertTrue( + "Should have timeout in 120 milli seconds " + timeTakenInMillis, timeTakenInMillis < 130); + } + + @Test + public void testHttpShardHandlerWithResponse() { + HttpShardHandlerFactory httpShardHandlerFactory = new HttpShardHandlerFactory(); + HttpShardHandler shardHandler = (HttpShardHandler) httpShardHandlerFactory.getShardHandler(); + + long startTime = System.nanoTime(); + long timeAllowedInMillis = -1; + shardHandler.setPendingRequest(1); + + ShardResponse shardResponse = new ShardResponse(); + shardResponse.setShard("shard_1"); + ShardRequest shardRequest = new ShardRequest(); + shardRequest.actualShards = new String[] {"shard_1"}; + shardResponse.setShardRequest(shardRequest); + + Thread thread = + new Thread( + () -> { + shardHandler.setResponse(shardResponse); + }); + thread.start(); + ShardResponse gotResponse = + shardHandler.takeCompletedIncludingErrorsWithTimeout(timeAllowedInMillis); + + assertEquals(shardResponse, gotResponse); + + long timeTakenInMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime); + assertTrue( + "Should have taken less than 100 milli seconds " + timeTakenInMillis, + timeTakenInMillis < 100); + } } From 5a31ebe3e2d8733aee982c6abc07124ab87a67ea Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 28 May 2024 11:48:22 -0700 Subject: [PATCH 03/12] fixed test --- .../solr/handler/component/TrackingShardHandlerFactory.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java b/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java index d7bfc961b77..83e4d093e07 100644 --- a/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java +++ b/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java @@ -103,6 +103,11 @@ public ShardResponse takeCompletedIncludingErrors() { return wrapped.takeCompletedIncludingErrors(); } + @Override + public ShardResponse takeCompletedIncludingErrorsWithTimeout (long maxAllowedTimeInMillis) { + return wrapped.takeCompletedIncludingErrors(); + } + @Override public ShardResponse takeCompletedOrError() { return wrapped.takeCompletedOrError(); From 4c023c368bd0cc5a0cdeb31032977c0c9f2d3f4d Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 28 May 2024 13:07:15 -0700 Subject: [PATCH 04/12] updated --- .../handler/component/HttpShardHandler.java | 2 +- .../TestHttpShardHandlerFactory.java | 26 ++++++++----------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java index f9501c739ee..3a702fbc0e5 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java @@ -246,7 +246,7 @@ private ShardResponse take(boolean bailOnError, long maxAllowedTimeInMillis) { if (maxAllowedTimeInMillis > 0) { deadline += TimeUnit.MILLISECONDS.toNanos(maxAllowedTimeInMillis); } else { - deadline = Long.MAX_VALUE; + deadline = System.nanoTime() + TimeUnit.DAYS.toNanos(1); } while (pending.get() > 0) { diff --git a/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java b/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java index 737803186b0..0ee465f4db1 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java +++ b/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java @@ -27,11 +27,13 @@ import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.client.solrj.impl.LBSolrClient; import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.common.cloud.ClusterState; +import org.apache.solr.common.util.ExecutorUtil; import org.apache.solr.core.CoreContainer; import org.hamcrest.MatcherAssert; import org.junit.AfterClass; @@ -168,14 +170,12 @@ public void testHttpShardHandlerTimeout() { ShardResponse shardResponse = shardHandler.takeCompletedIncludingErrorsWithTimeout(timeAllowedInMillis); - assertEquals(null, shardResponse); + assertNull(shardResponse); long timeTakenInMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime); assertTrue( "Should have taken more than 100 milli seconds " + timeTakenInMillis, - timeTakenInMillis > 100); - assertTrue( - "Should have timeout in 120 milli seconds " + timeTakenInMillis, timeTakenInMillis < 130); + timeTakenInMillis >= timeAllowedInMillis); } @Test @@ -193,20 +193,16 @@ public void testHttpShardHandlerWithResponse() { shardRequest.actualShards = new String[] {"shard_1"}; shardResponse.setShardRequest(shardRequest); - Thread thread = - new Thread( - () -> { - shardHandler.setResponse(shardResponse); - }); - thread.start(); + ExecutorService exec = ExecutorUtil.newMDCAwareCachedThreadPool("timeAllowedTest"); + try { + exec.submit(() -> shardHandler.setResponse(shardResponse)); + + } finally { + ExecutorUtil.shutdownAndAwaitTermination(exec); + } ShardResponse gotResponse = shardHandler.takeCompletedIncludingErrorsWithTimeout(timeAllowedInMillis); assertEquals(shardResponse, gotResponse); - - long timeTakenInMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime); - assertTrue( - "Should have taken less than 100 milli seconds " + timeTakenInMillis, - timeTakenInMillis < 100); } } From 6a4f7495fb3184d2a59c14349ca08d646eb8c27d Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 28 May 2024 13:13:05 -0700 Subject: [PATCH 05/12] spotlessApply --- .../solr/handler/component/TrackingShardHandlerFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java b/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java index 83e4d093e07..2bd588a887a 100644 --- a/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java +++ b/solr/test-framework/src/java/org/apache/solr/handler/component/TrackingShardHandlerFactory.java @@ -104,7 +104,7 @@ public ShardResponse takeCompletedIncludingErrors() { } @Override - public ShardResponse takeCompletedIncludingErrorsWithTimeout (long maxAllowedTimeInMillis) { + public ShardResponse takeCompletedIncludingErrorsWithTimeout(long maxAllowedTimeInMillis) { return wrapped.takeCompletedIncludingErrors(); } From 37b82924f6b65f2231b51257a190108a86f8aef4 Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 28 May 2024 17:03:59 -0700 Subject: [PATCH 06/12] Added timeallowed end-to-end test --- .../apache/solr/TestTimeAllowedSearch.java | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java new file mode 100644 index 00000000000..9118136e2a5 --- /dev/null +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -0,0 +1,59 @@ +package org.apache.solr; + +import com.carrotsearch.randomizedtesting.generators.RandomStrings; +import org.apache.solr.client.solrj.SolrQuery; +import org.apache.solr.client.solrj.impl.CloudSolrClient; +import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.QueryRequest; +import org.apache.solr.client.solrj.request.UpdateRequest; +import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.cloud.MiniSolrCloudCluster; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.SolrInputDocument; +import org.apache.solr.common.params.CommonParams; +import org.apache.solr.core.NodeRoles; +import org.apache.solr.embedded.JettySolrRunner; + +import java.util.List; +import java.util.Random; + +public class TestTimeAllowedSearch extends SolrCloudTestCase { + + public void testTimeAllowed() throws Exception { + MiniSolrCloudCluster cluster = + configureCluster(4).addConfig("conf", configset("cloud-minimal")).configure(); + try { + CloudSolrClient client = cluster.getSolrClient(); + String COLLECTION_NAME = "test_coll"; + CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1) + .process(cluster.getSolrClient()); + cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2); + UpdateRequest ur = new UpdateRequest(); + Random rd = new Random(); + for (int i = 0; i < 100; i++) { + SolrInputDocument doc = new SolrInputDocument(); + doc.addField("id", "" + i); + int min = rd.nextInt(100); + final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), min, min + 10); + doc.setField("subject_s", s); + ur.add(doc); + } + + ur.commit(client, COLLECTION_NAME); + + SolrQuery query = new SolrQuery(); + query.setQuery("subject_s:*a*"); + query.set(CommonParams.TIME_ALLOWED, 1); + QueryResponse response = client.query(COLLECTION_NAME, query); + assertTrue("Should not have found any doc as timeallowed is 1ms ", response.getResults().getNumFound() == 0); + + query = new SolrQuery(); + query.setQuery("subject_s:*b*"); + response = client.query(COLLECTION_NAME, query); + System.out.println("response " + response); + assertTrue("Should have found few docs as timeallowed is unlimited ", response.getResults().getNumFound() > 0); + } finally { + cluster.shutdown(); + } + } +} From ce88d5d74e188bbc9a1d9f03dbe13015ad3e70bf Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 28 May 2024 17:06:24 -0700 Subject: [PATCH 07/12] spotless --- .../apache/solr/TestTimeAllowedSearch.java | 77 +++++++++---------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java index 9118136e2a5..507b4967fd6 100644 --- a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -1,59 +1,58 @@ package org.apache.solr; import com.carrotsearch.randomizedtesting.generators.RandomStrings; +import java.util.Random; import org.apache.solr.client.solrj.SolrQuery; import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; -import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.request.UpdateRequest; import org.apache.solr.client.solrj.response.QueryResponse; import org.apache.solr.cloud.MiniSolrCloudCluster; import org.apache.solr.cloud.SolrCloudTestCase; import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.params.CommonParams; -import org.apache.solr.core.NodeRoles; -import org.apache.solr.embedded.JettySolrRunner; - -import java.util.List; -import java.util.Random; public class TestTimeAllowedSearch extends SolrCloudTestCase { - public void testTimeAllowed() throws Exception { - MiniSolrCloudCluster cluster = - configureCluster(4).addConfig("conf", configset("cloud-minimal")).configure(); - try { - CloudSolrClient client = cluster.getSolrClient(); - String COLLECTION_NAME = "test_coll"; - CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1) - .process(cluster.getSolrClient()); - cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2); - UpdateRequest ur = new UpdateRequest(); - Random rd = new Random(); - for (int i = 0; i < 100; i++) { - SolrInputDocument doc = new SolrInputDocument(); - doc.addField("id", "" + i); - int min = rd.nextInt(100); - final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), min, min + 10); - doc.setField("subject_s", s); - ur.add(doc); - } + public void testTimeAllowed() throws Exception { + MiniSolrCloudCluster cluster = + configureCluster(4).addConfig("conf", configset("cloud-minimal")).configure(); + try { + CloudSolrClient client = cluster.getSolrClient(); + String COLLECTION_NAME = "test_coll"; + CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1) + .process(cluster.getSolrClient()); + cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2); + UpdateRequest ur = new UpdateRequest(); + Random rd = new Random(); + for (int i = 0; i < 100; i++) { + SolrInputDocument doc = new SolrInputDocument(); + doc.addField("id", "" + i); + int min = rd.nextInt(100); + final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), min, min + 10); + doc.setField("subject_s", s); + ur.add(doc); + } - ur.commit(client, COLLECTION_NAME); + ur.commit(client, COLLECTION_NAME); - SolrQuery query = new SolrQuery(); - query.setQuery("subject_s:*a*"); - query.set(CommonParams.TIME_ALLOWED, 1); - QueryResponse response = client.query(COLLECTION_NAME, query); - assertTrue("Should not have found any doc as timeallowed is 1ms ", response.getResults().getNumFound() == 0); + SolrQuery query = new SolrQuery(); + query.setQuery("subject_s:*a*"); + query.set(CommonParams.TIME_ALLOWED, 1); + QueryResponse response = client.query(COLLECTION_NAME, query); + assertTrue( + "Should not have found any doc as timeallowed is 1ms ", + response.getResults().getNumFound() == 0); - query = new SolrQuery(); - query.setQuery("subject_s:*b*"); - response = client.query(COLLECTION_NAME, query); - System.out.println("response " + response); - assertTrue("Should have found few docs as timeallowed is unlimited ", response.getResults().getNumFound() > 0); - } finally { - cluster.shutdown(); - } + query = new SolrQuery(); + query.setQuery("subject_s:*b*"); + response = client.query(COLLECTION_NAME, query); + System.out.println("response " + response); + assertTrue( + "Should have found few docs as timeallowed is unlimited ", + response.getResults().getNumFound() > 0); + } finally { + cluster.shutdown(); } + } } From 48061b2d3d3719007e657ed01cfaea8dd823db6f Mon Sep 17 00:00:00 2001 From: hitesh Date: Tue, 28 May 2024 20:21:07 -0700 Subject: [PATCH 08/12] fix partial response issue; added test --- .../handler/component/HttpShardHandler.java | 4 ++- .../apache/solr/TestTimeAllowedSearch.java | 23 +++++++++++-- .../TestHttpShardHandlerFactory.java | 34 ++++++++++++++++++- 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java index 3a702fbc0e5..652dd0bc3d7 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java +++ b/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java @@ -249,10 +249,11 @@ private ShardResponse take(boolean bailOnError, long maxAllowedTimeInMillis) { deadline = System.nanoTime() + TimeUnit.DAYS.toNanos(1); } + ShardResponse previousResponse = null; while (pending.get() > 0) { long waitTime = deadline - System.nanoTime(); ShardResponse rsp = responses.poll(waitTime, TimeUnit.NANOSECONDS); - if (rsp == null) return null; + if (rsp == null) return previousResponse; responseCancellableMap.remove(rsp); pending.decrementAndGet(); @@ -263,6 +264,7 @@ private ShardResponse take(boolean bailOnError, long maxAllowedTimeInMillis) { // for a request was received. Otherwise we might return the same // request more than once. rsp.getShardRequest().responses.add(rsp); + previousResponse = rsp; if (rsp.getShardRequest().responses.size() == rsp.getShardRequest().actualShards.length) { return rsp; } diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java index 507b4967fd6..7f8e987d20b 100644 --- a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -11,18 +11,19 @@ import org.apache.solr.cloud.SolrCloudTestCase; import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.params.CommonParams; +import org.apache.solr.common.params.ShardParams; public class TestTimeAllowedSearch extends SolrCloudTestCase { public void testTimeAllowed() throws Exception { MiniSolrCloudCluster cluster = - configureCluster(4).addConfig("conf", configset("cloud-minimal")).configure(); + configureCluster(2).addConfig("conf", configset("cloud-minimal")).configure(); try { CloudSolrClient client = cluster.getSolrClient(); String COLLECTION_NAME = "test_coll"; - CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1) + CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 4, 1) .process(cluster.getSolrClient()); - cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2); + cluster.waitForActiveCollection(COLLECTION_NAME, 4, 4); UpdateRequest ur = new UpdateRequest(); Random rd = new Random(); for (int i = 0; i < 100; i++) { @@ -51,6 +52,22 @@ public void testTimeAllowed() throws Exception { assertTrue( "Should have found few docs as timeallowed is unlimited ", response.getResults().getNumFound() > 0); + + long totalResults = response.getResults().getNumFound(); + + cluster.getJettySolrRunner(1).stop(); + + query = new SolrQuery(); + // executing same query but one node is down + query.setQuery("subject_s:*b*"); + query.set(ShardParams.SHARDS_TOLERANT, "true"); + query.set(CommonParams.TIME_ALLOWED, 100); + response = client.query(COLLECTION_NAME, query); + System.out.println("response " + response); + assertTrue( + "Should have found less docs as one node is stopped ", + response.getResults().getNumFound() < totalResults); + } finally { cluster.shutdown(); } diff --git a/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java b/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java index 0ee465f4db1..b28ec96ef0a 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java +++ b/solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java @@ -183,23 +183,55 @@ public void testHttpShardHandlerWithResponse() { HttpShardHandlerFactory httpShardHandlerFactory = new HttpShardHandlerFactory(); HttpShardHandler shardHandler = (HttpShardHandler) httpShardHandlerFactory.getShardHandler(); - long startTime = System.nanoTime(); long timeAllowedInMillis = -1; + // setting one pending request. shardHandler.setPendingRequest(1); ShardResponse shardResponse = new ShardResponse(); shardResponse.setShard("shard_1"); ShardRequest shardRequest = new ShardRequest(); + // one shard shardRequest.actualShards = new String[] {"shard_1"}; shardResponse.setShardRequest(shardRequest); ExecutorService exec = ExecutorUtil.newMDCAwareCachedThreadPool("timeAllowedTest"); try { + // generating shardresponse for one shard exec.submit(() -> shardHandler.setResponse(shardResponse)); + } finally { + ExecutorUtil.shutdownAndAwaitTermination(exec); + } + ShardResponse gotResponse = + shardHandler.takeCompletedIncludingErrorsWithTimeout(timeAllowedInMillis); + assertEquals(shardResponse, gotResponse); + } + + @Test + public void testHttpShardHandlerWithPartialResponse() { + HttpShardHandlerFactory httpShardHandlerFactory = new HttpShardHandlerFactory(); + HttpShardHandler shardHandler = (HttpShardHandler) httpShardHandlerFactory.getShardHandler(); + + long timeAllowedInMillis = 100; + // setting two pending requests. + shardHandler.setPendingRequest(2); + + ShardResponse shardResponse = new ShardResponse(); + shardResponse.setShard("shard_1"); + ShardRequest shardRequest = new ShardRequest(); + // two shards + shardRequest.actualShards = new String[] {"shard_1", "shard_2"}; + shardResponse.setShardRequest(shardRequest); + + ExecutorService exec = ExecutorUtil.newMDCAwareCachedThreadPool("timeAllowedTest"); + try { + // generating shardresponse for one shard only + exec.submit(() -> shardHandler.setResponse(shardResponse)); } finally { ExecutorUtil.shutdownAndAwaitTermination(exec); } + + // partial response ShardResponse gotResponse = shardHandler.takeCompletedIncludingErrorsWithTimeout(timeAllowedInMillis); From b5e5bbe211437e16bea7757f3b1b5db02b1681f1 Mon Sep 17 00:00:00 2001 From: hitesh Date: Thu, 30 May 2024 10:49:13 -0700 Subject: [PATCH 09/12] Updated test based on shard1 load vs shard2 load(number of docs) --- .../apache/solr/TestTimeAllowedSearch.java | 67 ++++++++++--------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java index 7f8e987d20b..207d154b41b 100644 --- a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -1,6 +1,9 @@ package org.apache.solr; +import com.carrotsearch.randomizedtesting.RandomizedRunner; import com.carrotsearch.randomizedtesting.generators.RandomStrings; + +import java.util.Locale; import java.util.Random; import org.apache.solr.client.solrj.SolrQuery; import org.apache.solr.client.solrj.impl.CloudSolrClient; @@ -15,59 +18,63 @@ public class TestTimeAllowedSearch extends SolrCloudTestCase { + /** + * This test demonstrates timeAllowed expectation at @{@link org.apache.solr.handler.component.HttpShardHandler} level + * This test creates collection with 'implicit` router, which has two shards + * shard_1 has 100000 docs, so that query should take some time + * shard_2 has only 1 doc to demonstrate the HttpSHardHandler timeout + * Then it execute substring query with TIME_ALLOWED 50, assuming this query will time out on shard_1 + * @throws Exception + */ public void testTimeAllowed() throws Exception { MiniSolrCloudCluster cluster = configureCluster(2).addConfig("conf", configset("cloud-minimal")).configure(); try { CloudSolrClient client = cluster.getSolrClient(); String COLLECTION_NAME = "test_coll"; - CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 4, 1) - .process(cluster.getSolrClient()); - cluster.waitForActiveCollection(COLLECTION_NAME, 4, 4); + CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1) + .setRouterName("implicit").setShards("shard_1,shard_2") + .process(cluster.getSolrClient()); + cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2); UpdateRequest ur = new UpdateRequest(); - Random rd = new Random(); - for (int i = 0; i < 100; i++) { + for (int i = 0; i < 100000; i++) { SolrInputDocument doc = new SolrInputDocument(); doc.addField("id", "" + i); - int min = rd.nextInt(100); - final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), min, min + 10); + final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) + .toLowerCase(Locale.ROOT); doc.setField("subject_s", s); + doc.setField("_route_", "shard_1"); + ur.add(doc); + } + + for (int i = 0; i < 1; i++) { + SolrInputDocument doc = new SolrInputDocument(); + doc.addField("id", "" + i); + final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) + .toLowerCase(Locale.ROOT); + doc.setField("subject_s", s); + doc.setField("_route_", "shard_2"); ur.add(doc); } ur.commit(client, COLLECTION_NAME); SolrQuery query = new SolrQuery(); - query.setQuery("subject_s:*a*"); - query.set(CommonParams.TIME_ALLOWED, 1); + query.setQuery("subject_s:*abcd*"); + query.set(CommonParams.TIME_ALLOWED, 50); + query.set(ShardParams.SHARDS_TOLERANT, "true"); QueryResponse response = client.query(COLLECTION_NAME, query); assertTrue( - "Should not have found any doc as timeallowed is 1ms ", - response.getResults().getNumFound() == 0); + "Should have found 1/0 doc as timeallowed is 50ms found:" + response.getResults().getNumFound() , + response.getResults().getNumFound() <= 1); query = new SolrQuery(); - query.setQuery("subject_s:*b*"); - response = client.query(COLLECTION_NAME, query); - System.out.println("response " + response); - assertTrue( - "Should have found few docs as timeallowed is unlimited ", - response.getResults().getNumFound() > 0); - - long totalResults = response.getResults().getNumFound(); - - cluster.getJettySolrRunner(1).stop(); - - query = new SolrQuery(); - // executing same query but one node is down - query.setQuery("subject_s:*b*"); + query.setQuery("subject_s:*abcd*"); query.set(ShardParams.SHARDS_TOLERANT, "true"); - query.set(CommonParams.TIME_ALLOWED, 100); response = client.query(COLLECTION_NAME, query); - System.out.println("response " + response); assertTrue( - "Should have found less docs as one node is stopped ", - response.getResults().getNumFound() < totalResults); - + "Should have found few docs as timeallowed is unlimited ", + response.getResults().getNumFound() >= 0); } finally { cluster.shutdown(); } From 11a49222edd7dd869ee81fdfacc439e720e49fb4 Mon Sep 17 00:00:00 2001 From: hitesh Date: Thu, 30 May 2024 11:16:40 -0700 Subject: [PATCH 10/12] spotlessApply --- .../apache/solr/TestTimeAllowedSearch.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java index 207d154b41b..56d06e0434e 100644 --- a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -1,10 +1,7 @@ package org.apache.solr; -import com.carrotsearch.randomizedtesting.RandomizedRunner; import com.carrotsearch.randomizedtesting.generators.RandomStrings; - import java.util.Locale; -import java.util.Random; import org.apache.solr.client.solrj.SolrQuery; import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; @@ -19,12 +16,11 @@ public class TestTimeAllowedSearch extends SolrCloudTestCase { /** - * This test demonstrates timeAllowed expectation at @{@link org.apache.solr.handler.component.HttpShardHandler} level - * This test creates collection with 'implicit` router, which has two shards - * shard_1 has 100000 docs, so that query should take some time - * shard_2 has only 1 doc to demonstrate the HttpSHardHandler timeout - * Then it execute substring query with TIME_ALLOWED 50, assuming this query will time out on shard_1 - * @throws Exception + * This test demonstrates timeAllowed expectation at @{@link + * org.apache.solr.handler.component.HttpShardHandler} level This test creates collection with + * 'implicit` router, which has two shards shard_1 has 100000 docs, so that query should take some + * time shard_2 has only 1 doc to demonstrate the HttpSHardHandler timeout Then it execute + * substring query with TIME_ALLOWED 50, assuming this query will time out on shard_1 */ public void testTimeAllowed() throws Exception { MiniSolrCloudCluster cluster = @@ -33,14 +29,16 @@ public void testTimeAllowed() throws Exception { CloudSolrClient client = cluster.getSolrClient(); String COLLECTION_NAME = "test_coll"; CollectionAdminRequest.createCollection(COLLECTION_NAME, "conf", 2, 1) - .setRouterName("implicit").setShards("shard_1,shard_2") - .process(cluster.getSolrClient()); + .setRouterName("implicit") + .setShards("shard_1,shard_2") + .process(cluster.getSolrClient()); cluster.waitForActiveCollection(COLLECTION_NAME, 2, 2); UpdateRequest ur = new UpdateRequest(); for (int i = 0; i < 100000; i++) { SolrInputDocument doc = new SolrInputDocument(); doc.addField("id", "" + i); - final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) + final String s = + RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) .toLowerCase(Locale.ROOT); doc.setField("subject_s", s); doc.setField("_route_", "shard_1"); @@ -50,7 +48,8 @@ public void testTimeAllowed() throws Exception { for (int i = 0; i < 1; i++) { SolrInputDocument doc = new SolrInputDocument(); doc.addField("id", "" + i); - final String s = RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) + final String s = + RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) .toLowerCase(Locale.ROOT); doc.setField("subject_s", s); doc.setField("_route_", "shard_2"); @@ -65,7 +64,8 @@ public void testTimeAllowed() throws Exception { query.set(ShardParams.SHARDS_TOLERANT, "true"); QueryResponse response = client.query(COLLECTION_NAME, query); assertTrue( - "Should have found 1/0 doc as timeallowed is 50ms found:" + response.getResults().getNumFound() , + "Should have found 1/0 doc as timeallowed is 50ms found:" + + response.getResults().getNumFound(), response.getResults().getNumFound() <= 1); query = new SolrQuery(); From b9613754f5651f8edd853371d0c269c53eb1e115 Mon Sep 17 00:00:00 2001 From: hitesh Date: Thu, 30 May 2024 12:59:37 -0700 Subject: [PATCH 11/12] tunning test --- .../test/org/apache/solr/TestTimeAllowedSearch.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java index 56d06e0434e..eb631221c6a 100644 --- a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -58,18 +58,24 @@ public void testTimeAllowed() throws Exception { ur.commit(client, COLLECTION_NAME); + // warm up query SolrQuery query = new SolrQuery(); query.setQuery("subject_s:*abcd*"); - query.set(CommonParams.TIME_ALLOWED, 50); query.set(ShardParams.SHARDS_TOLERANT, "true"); QueryResponse response = client.query(COLLECTION_NAME, query); + + query = new SolrQuery(); + query.setQuery("subject_s:*xyz*"); + query.set(CommonParams.TIME_ALLOWED, 25); + query.set(ShardParams.SHARDS_TOLERANT, "true"); + response = client.query(COLLECTION_NAME, query); assertTrue( "Should have found 1/0 doc as timeallowed is 50ms found:" + response.getResults().getNumFound(), response.getResults().getNumFound() <= 1); query = new SolrQuery(); - query.setQuery("subject_s:*abcd*"); + query.setQuery("subject_s:*xyz*"); query.set(ShardParams.SHARDS_TOLERANT, "true"); response = client.query(COLLECTION_NAME, query); assertTrue( From 738fc631bc89712a7363247fc73d7fbfc4c2bb77 Mon Sep 17 00:00:00 2001 From: hitesh Date: Fri, 31 May 2024 06:26:06 -0700 Subject: [PATCH 12/12] more strict condition in test --- .../apache/solr/TestTimeAllowedSearch.java | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java index eb631221c6a..580c603711a 100644 --- a/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java +++ b/solr/core/src/test/org/apache/solr/TestTimeAllowedSearch.java @@ -45,16 +45,18 @@ public void testTimeAllowed() throws Exception { ur.add(doc); } - for (int i = 0; i < 1; i++) { - SolrInputDocument doc = new SolrInputDocument(); - doc.addField("id", "" + i); - final String s = - RandomStrings.randomAsciiLettersOfLengthBetween(random(), 10, 100) - .toLowerCase(Locale.ROOT); - doc.setField("subject_s", s); - doc.setField("_route_", "shard_2"); - ur.add(doc); - } + // adding "abc" in each shard as we will have query *abc* + SolrInputDocument doc = new SolrInputDocument(); + doc.addField("id", "" + 10000); + doc.setField("subject_s", "abc"); + doc.setField("_route_", "shard_2"); + ur.add(doc); + + doc = new SolrInputDocument(); + doc.addField("id", "" + 100001); + doc.setField("subject_s", "abc"); + doc.setField("_route_", "shard_1"); + ur.add(doc); ur.commit(client, COLLECTION_NAME); @@ -65,22 +67,22 @@ public void testTimeAllowed() throws Exception { QueryResponse response = client.query(COLLECTION_NAME, query); query = new SolrQuery(); - query.setQuery("subject_s:*xyz*"); + query.setQuery("subject_s:*abc*"); query.set(CommonParams.TIME_ALLOWED, 25); query.set(ShardParams.SHARDS_TOLERANT, "true"); response = client.query(COLLECTION_NAME, query); assertTrue( - "Should have found 1/0 doc as timeallowed is 50ms found:" + "Should have found 1 doc (shard_2) as timeallowed is 25ms found:" + response.getResults().getNumFound(), - response.getResults().getNumFound() <= 1); + response.getResults().getNumFound() == 1); query = new SolrQuery(); - query.setQuery("subject_s:*xyz*"); + query.setQuery("subject_s:*abc*"); query.set(ShardParams.SHARDS_TOLERANT, "true"); response = client.query(COLLECTION_NAME, query); assertTrue( "Should have found few docs as timeallowed is unlimited ", - response.getResults().getNumFound() >= 0); + response.getResults().getNumFound() > 1); } finally { cluster.shutdown(); }