dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1139500432
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -332,6 +335,11 @@ public Builder withRetryExpiryTime(long expiryTime,
TimeUnit unit) {
return this;
}
+ /** Sets the default collection for request. */
+ public Builder withDefaultCollection(String collection) {
Review Comment:
Of course, this is the meat of the change. A new builder option.
I was thinking of @ishan's feedback on my other straw-man PR. I think we
should consider pivoting the idea here from a "default" collection to a
constraint. It would be confusing to specify this setting and yet use the
client for other collections; no? This would more likely be a bug rather than
intentional by a user, thus implementing as a limit probably helps the user
more? It'd be easy to implement the constraint; I could show if we come to
like this idea. It wouldn't be a hard guarantee -- not a security control, as
there are ways around it. WDYT?
##########
solr/core/src/test/org/apache/solr/cloud/TestCloudSearcherWarming.java:
##########
@@ -311,14 +307,11 @@ public boolean onStateChanged(Set<String> liveNodes,
DocCollection collectionSta
log.error(
"registered searcher not null, maxdocs = {}",
registeredSearcher.get().maxDoc());
+ registeredSearcher.decref();
Review Comment:
You tried to improve something but it's no longer correct. Can't decref
then get! decref last (when done).
##########
solr/core/src/test/org/apache/solr/cloud/SplitShardTest.java:
##########
@@ -374,10 +372,11 @@ private boolean testColl(
@Test
public void testLiveSplit() throws Exception {
- // Debugging tips: if this fails, it may be easier to debug by lowering
the number fo threads to
+ // Debugging tips: if this fails, it may be easier to debug by lowering
the number of threads to
// 1 and looping the test until you get another failure. You may need to
further instrument
// things like DistributedZkUpdateProcessor to display the cluster state
for the collection,
- // etc. Using more threads increases the chance to hit a concurrency bug,
but too many threads
+ // etc. Using more threads increases the chance of hitting a concurrency
bug, but too many
+ // threads
// can overwhelm single-threaded buffering replay after the low level
index split and result in
Review Comment:
reflow
##########
solr/solr-ref-guide/modules/indexing-guide/examples/IndexingNestedDocuments.java:
##########
@@ -66,89 +69,94 @@ public static SolrClient getSolrClient() {
*/
public void testIndexingAnonKids() throws Exception {
final String collection = "test_anon";
+
CollectionAdminRequest.createCollection(collection, ANON_KIDS_CONFIG, 1, 1)
.setPerReplicaState(SolrCloudTestCase.USE_PER_REPLICA_STATE)
.process(cluster.getSolrClient());
- cluster.getSolrClient().setDefaultCollection(collection);
+
+ // configure the client with the default collection name, to simplify our
example below.
+ IndexingNestedDocuments.clientUsedInSolrJExample =
+
cluster.basicSolrClientBuilder().withDefaultCollection(collection).build();
//
- // DO NOT MODIFY THESE EXAMPLE DOCS WITH OUT MAKING THE SAME CHANGES TO
THE JSON AND XML
- // EQUIVILENT EXAMPLES IN 'indexing-nested-documents.adoc'
+ // DO NOT MODIFY THESE EXAMPLE DOCS WITHOUT MAKING THE SAME CHANGES TO THE
JSON AND XML
+ // EQUIVALENT EXAMPLES IN 'indexing-nested-documents.adoc'
Review Comment:
Did that happen?
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -716,6 +718,12 @@ public void shutdown() throws Exception {
try {
IOUtils.closeQuietly(solrClient);
+
+ solrClientForCollectionCache.values().parallelStream()
Review Comment:
should be clear'ed too
##########
solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java:
##########
@@ -51,105 +52,108 @@ public static void setupCluster() throws Exception {
@Test
public void testConcurrentQueries() throws Exception {
- CloudSolrClient client = cluster.getSolrClient();
- client.setDefaultCollection(FIRST_COLLECTION);
-
- CollectionAdminRequest.createCollection(FIRST_COLLECTION, 1,
1).process(client);
- cluster.waitForActiveCollection(FIRST_COLLECTION, 1, 1);
-
- SolrDispatchFilter solrDispatchFilter =
cluster.getJettySolrRunner(0).getSolrDispatchFilter();
-
- RateLimiterConfig rateLimiterConfig =
- new RateLimiterConfig(
- SolrRequest.SolrRequestType.QUERY,
- true,
- 1,
- DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
- 5 /* allowedRequests */,
- true /* isSlotBorrowing */);
- // We are fine with a null FilterConfig here since we ensure that
MockBuilder never invokes its
- // parent here
- RateLimitManager.Builder builder =
- new MockBuilder(
- null /* dummy SolrZkClient */, new
MockRequestRateLimiter(rateLimiterConfig));
- RateLimitManager rateLimitManager = builder.build();
-
- solrDispatchFilter.replaceRateLimitManager(rateLimitManager);
-
- int numDocs = TEST_NIGHTLY ? 10000 : 100;
-
- processTest(client, numDocs, 350 /* number of queries */);
-
- MockRequestRateLimiter mockQueryRateLimiter =
- (MockRequestRateLimiter)
-
rateLimitManager.getRequestRateLimiter(SolrRequest.SolrRequestType.QUERY);
-
- assertEquals(350, mockQueryRateLimiter.incomingRequestCount.get());
-
- assertTrue(mockQueryRateLimiter.acceptedNewRequestCount.get() > 0);
- assertTrue(
- (mockQueryRateLimiter.acceptedNewRequestCount.get()
- == mockQueryRateLimiter.incomingRequestCount.get()
- || mockQueryRateLimiter.rejectedRequestCount.get() > 0));
- assertEquals(
- mockQueryRateLimiter.incomingRequestCount.get(),
- mockQueryRateLimiter.acceptedNewRequestCount.get()
- + mockQueryRateLimiter.rejectedRequestCount.get());
+ try (CloudSolrClient client =
+
cluster.basicSolrClientBuilder().withDefaultCollection(FIRST_COLLECTION).build())
{
+
+ CollectionAdminRequest.createCollection(FIRST_COLLECTION, 1,
1).process(client);
+ cluster.waitForActiveCollection(FIRST_COLLECTION, 1, 1);
+
+ SolrDispatchFilter solrDispatchFilter =
cluster.getJettySolrRunner(0).getSolrDispatchFilter();
+
+ RateLimiterConfig rateLimiterConfig =
+ new RateLimiterConfig(
+ SolrRequest.SolrRequestType.QUERY,
+ true,
+ 1,
+ DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
+ 5 /* allowedRequests */,
+ true /* isSlotBorrowing */);
+ // We are fine with a null FilterConfig here since we ensure that
MockBuilder never invokes
+ // its
+ // parent here
+ RateLimitManager.Builder builder =
+ new MockBuilder(
+ null /* dummy SolrZkClient */, new
MockRequestRateLimiter(rateLimiterConfig));
+ RateLimitManager rateLimitManager = builder.build();
+
+ solrDispatchFilter.replaceRateLimitManager(rateLimitManager);
+
+ int numDocs = TEST_NIGHTLY ? 10000 : 100;
+
+ processTest(client, numDocs, 350 /* number of queries */);
+
+ MockRequestRateLimiter mockQueryRateLimiter =
+ (MockRequestRateLimiter)
+
rateLimitManager.getRequestRateLimiter(SolrRequest.SolrRequestType.QUERY);
+
+ assertEquals(350, mockQueryRateLimiter.incomingRequestCount.get());
+
+ assertTrue(mockQueryRateLimiter.acceptedNewRequestCount.get() > 0);
+ assertTrue(
+ (mockQueryRateLimiter.acceptedNewRequestCount.get()
+ == mockQueryRateLimiter.incomingRequestCount.get()
+ || mockQueryRateLimiter.rejectedRequestCount.get() > 0));
+ assertEquals(
+ mockQueryRateLimiter.incomingRequestCount.get(),
+ mockQueryRateLimiter.acceptedNewRequestCount.get()
+ + mockQueryRateLimiter.rejectedRequestCount.get());
+ }
}
@Nightly
public void testSlotBorrowing() throws Exception {
- CloudSolrClient client = cluster.getSolrClient();
- client.setDefaultCollection(SECOND_COLLECTION);
-
- CollectionAdminRequest.createCollection(SECOND_COLLECTION, 1,
1).process(client);
- cluster.waitForActiveCollection(SECOND_COLLECTION, 1, 1);
-
- SolrDispatchFilter solrDispatchFilter =
cluster.getJettySolrRunner(0).getSolrDispatchFilter();
-
- RateLimiterConfig queryRateLimiterConfig =
- new RateLimiterConfig(
- SolrRequest.SolrRequestType.QUERY,
- true,
- 1,
- DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
- 5 /* allowedRequests */,
- true /* isSlotBorrowing */);
- RateLimiterConfig indexRateLimiterConfig =
- new RateLimiterConfig(
- SolrRequest.SolrRequestType.UPDATE,
- true,
- 1,
- DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
- 5 /* allowedRequests */,
- true /* isSlotBorrowing */);
- // We are fine with a null FilterConfig here since we ensure that
MockBuilder never invokes its
- // parent
- RateLimitManager.Builder builder =
- new MockBuilder(
- null /*dummy SolrZkClient */,
- new MockRequestRateLimiter(queryRateLimiterConfig),
- new MockRequestRateLimiter(indexRateLimiterConfig));
- RateLimitManager rateLimitManager = builder.build();
-
- solrDispatchFilter.replaceRateLimitManager(rateLimitManager);
-
- int numDocs = 10000;
-
- processTest(client, numDocs, 400 /* Number of queries */);
-
- MockRequestRateLimiter mockIndexRateLimiter =
- (MockRequestRateLimiter)
-
rateLimitManager.getRequestRateLimiter(SolrRequest.SolrRequestType.UPDATE);
-
- assertTrue(
- "Incoming slots borrowed count did not match. Expected > 0 incoming "
- + mockIndexRateLimiter.borrowedSlotCount.get(),
- mockIndexRateLimiter.borrowedSlotCount.get() > 0);
+ try (CloudSolrClient client =
+
cluster.basicSolrClientBuilder().withDefaultCollection(SECOND_COLLECTION).build())
{
+
+ CollectionAdminRequest.createCollection(SECOND_COLLECTION, 1,
1).process(client);
+ cluster.waitForActiveCollection(SECOND_COLLECTION, 1, 1);
+
+ SolrDispatchFilter solrDispatchFilter =
cluster.getJettySolrRunner(0).getSolrDispatchFilter();
+
+ RateLimiterConfig queryRateLimiterConfig =
+ new RateLimiterConfig(
+ SolrRequest.SolrRequestType.QUERY,
+ true,
+ 1,
+ DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
+ 5 /* allowedRequests */,
+ true /* isSlotBorrowing */);
+ RateLimiterConfig indexRateLimiterConfig =
+ new RateLimiterConfig(
+ SolrRequest.SolrRequestType.UPDATE,
+ true,
+ 1,
+ DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
+ 5 /* allowedRequests */,
+ true /* isSlotBorrowing */);
+ // We are fine with a null FilterConfig here since we ensure that
MockBuilder never invokes
+ // its
+ // parent
Review Comment:
reflow
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -156,11 +157,12 @@ public class MiniSolrCloudCluster {
private final boolean externalZkServer;
private final List<JettySolrRunner> jettys = new CopyOnWriteArrayList<>();
private final Path baseDir;
- private final CloudSolrClient solrClient;
+ private CloudSolrClient solrClient;
private final JettyConfig jettyConfig;
private final boolean trackJettyMetrics;
private final AtomicInteger nodeIds = new AtomicInteger();
+ private Map<String, CloudSolrClient> solrClientForCollectionCache = new
ConcurrentHashMap<>();
Review Comment:
I suggest "solrClientByCollection" -- shorter and the "by" part is more
typical (I think?) for maps to say what the key is.
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -305,8 +306,17 @@ private void doForceLeader(String collectionName, String
shard)
throws IOException, SolrServerException {
CollectionAdminRequest.ForceLeader forceLeader =
CollectionAdminRequest.forceLeaderElection(collectionName, shard);
+ boolean shardLeadersOnly = random().nextBoolean();
+ RandomizingCloudSolrClientBuilder builder =
Review Comment:
Again, lengthy types beg for use "var"
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java:
##########
@@ -148,10 +150,16 @@ private void
doSplitStaticIndexReplication(SolrIndexSplitter.SplitMethod splitMe
.waitForState(
collectionName, 30, TimeUnit.SECONDS,
SolrCloudTestCase.activeClusterShape(1, 1));
+ RandomizingCloudSolrClientBuilder builder =
Review Comment:
"var" please
##########
solr/core/src/test/org/apache/solr/client/solrj/impl/ConnectionReuseTest.java:
##########
@@ -85,15 +87,21 @@ private SolrClient buildClient(CloseableHttpClient
httpClient, URL url) {
case 1:
return getHttpSolrClient(url.toString() + "/" + COLLECTION,
httpClient);
case 2:
- CloudSolrClient client =
- getCloudSolrClient(
- cluster.getZkServer().getZkAddress(),
- random().nextBoolean(),
- httpClient,
- 30000,
- 60000);
- client.setDefaultCollection(COLLECTION);
- return client;
+ RandomizingCloudSolrClientBuilder builder =
Review Comment:
Screams for "var". Nitpicking I know
##########
solr/core/src/test/org/apache/solr/servlet/TestRequestRateLimiter.java:
##########
@@ -51,105 +52,108 @@ public static void setupCluster() throws Exception {
@Test
public void testConcurrentQueries() throws Exception {
- CloudSolrClient client = cluster.getSolrClient();
- client.setDefaultCollection(FIRST_COLLECTION);
-
- CollectionAdminRequest.createCollection(FIRST_COLLECTION, 1,
1).process(client);
- cluster.waitForActiveCollection(FIRST_COLLECTION, 1, 1);
-
- SolrDispatchFilter solrDispatchFilter =
cluster.getJettySolrRunner(0).getSolrDispatchFilter();
-
- RateLimiterConfig rateLimiterConfig =
- new RateLimiterConfig(
- SolrRequest.SolrRequestType.QUERY,
- true,
- 1,
- DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
- 5 /* allowedRequests */,
- true /* isSlotBorrowing */);
- // We are fine with a null FilterConfig here since we ensure that
MockBuilder never invokes its
- // parent here
- RateLimitManager.Builder builder =
- new MockBuilder(
- null /* dummy SolrZkClient */, new
MockRequestRateLimiter(rateLimiterConfig));
- RateLimitManager rateLimitManager = builder.build();
-
- solrDispatchFilter.replaceRateLimitManager(rateLimitManager);
-
- int numDocs = TEST_NIGHTLY ? 10000 : 100;
-
- processTest(client, numDocs, 350 /* number of queries */);
-
- MockRequestRateLimiter mockQueryRateLimiter =
- (MockRequestRateLimiter)
-
rateLimitManager.getRequestRateLimiter(SolrRequest.SolrRequestType.QUERY);
-
- assertEquals(350, mockQueryRateLimiter.incomingRequestCount.get());
-
- assertTrue(mockQueryRateLimiter.acceptedNewRequestCount.get() > 0);
- assertTrue(
- (mockQueryRateLimiter.acceptedNewRequestCount.get()
- == mockQueryRateLimiter.incomingRequestCount.get()
- || mockQueryRateLimiter.rejectedRequestCount.get() > 0));
- assertEquals(
- mockQueryRateLimiter.incomingRequestCount.get(),
- mockQueryRateLimiter.acceptedNewRequestCount.get()
- + mockQueryRateLimiter.rejectedRequestCount.get());
+ try (CloudSolrClient client =
+
cluster.basicSolrClientBuilder().withDefaultCollection(FIRST_COLLECTION).build())
{
+
+ CollectionAdminRequest.createCollection(FIRST_COLLECTION, 1,
1).process(client);
+ cluster.waitForActiveCollection(FIRST_COLLECTION, 1, 1);
+
+ SolrDispatchFilter solrDispatchFilter =
cluster.getJettySolrRunner(0).getSolrDispatchFilter();
+
+ RateLimiterConfig rateLimiterConfig =
+ new RateLimiterConfig(
+ SolrRequest.SolrRequestType.QUERY,
+ true,
+ 1,
+ DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS,
+ 5 /* allowedRequests */,
+ true /* isSlotBorrowing */);
+ // We are fine with a null FilterConfig here since we ensure that
MockBuilder never invokes
+ // its
+ // parent here
Review Comment:
reflow
##########
solr/core/src/test/org/apache/solr/search/join/TestCloudNestedDocsSort.java:
##########
@@ -131,7 +131,7 @@ public static void setupCluster() throws Exception {
@AfterClass
public static void cleanUpAfterClass() {
- client = null;
Review Comment:
In a number of places, you are changing code that nulled static fields to
not do so any more; only close the resource. But I think there is a retained
memory/heap concern? Maybe if we look carefully at the test run performance,
we'll see if this is an actual concern or not. Running with only a couple
threads would better ascertain this (vs more).
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -745,6 +753,30 @@ public CloudSolrClient getSolrClient() {
return solrClient;
}
+ public CloudSolrClient getSolrClientForCollection(String collectionName) {
Review Comment:
There is commonality here in what you are doing in this PR with the new Solr
TestRule, as seen by this method. In the TestRule, the "ForCollection" part is
omitted for brevity. WDYT? Also needs javadocs to tell the caller not to
close it; it's closed when the cluster is shutdown.
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -156,11 +157,12 @@ public class MiniSolrCloudCluster {
private final boolean externalZkServer;
private final List<JettySolrRunner> jettys = new CopyOnWriteArrayList<>();
private final Path baseDir;
- private final CloudSolrClient solrClient;
+ private CloudSolrClient solrClient;
private final JettyConfig jettyConfig;
private final boolean trackJettyMetrics;
private final AtomicInteger nodeIds = new AtomicInteger();
+ private Map<String, CloudSolrClient> solrClientForCollectionCache = new
ConcurrentHashMap<>();
Review Comment:
And can be final
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -156,11 +157,12 @@ public class MiniSolrCloudCluster {
private final boolean externalZkServer;
private final List<JettySolrRunner> jettys = new CopyOnWriteArrayList<>();
private final Path baseDir;
- private final CloudSolrClient solrClient;
+ private CloudSolrClient solrClient;
Review Comment:
Do we even need this field now that you added a map by collection? Use an
empty string as key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
