You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/12/29 16:03:31 UTC
[GitHub] [solr] epugh opened a new pull request, #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
epugh opened a new pull request, #1256:
URL: https://github.com/apache/solr/pull/1256
https://issues.apache.org/jira/browse/SOLR-10466
# Description
SolrClient's should be immutable, and right now for CloudSolrClient the defaultCollection is part of the state and changeable.
# Solution
Moving setDefaultCollection to Builder, and seeing what impact that has. It appears to mostly be a lot of clean up of tests?
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142859716
##########
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:
(done)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148546267
##########
solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java:
##########
@@ -563,25 +541,29 @@ protected void assertDocsExistInAllReplicas(
}
}
- protected SolrClient getHttpSolrClient(Replica replica, String coll) {
+ protected SolrClient getHttpSolrClient(Replica replica, String collection) {
Review Comment:
if it was `coll` everywhere in the code base, then I'd be a fan of `coll`. Since I see `collection`, i made that change.... One thing I thought about was going through and at least in the tests, we use `collection`, `testCollectionName` and `collectionName` and getting that to one value..
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh merged pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #1256:
URL: https://github.com/apache/solr/pull/1256
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1367461056
Okay, now I need some ideas @stillalex @dsmiley.... I've migrated to the Builder all the examples where we create a new SolrCloudClient, so it's easy to add the builder logic. The remaining ones all basically are reusing a SolrClient inherited from the base test class. For example, in `TestTrackingShardHandlerFactory` we have the method:
```
cluster.getSolrClient().setDefaultCollection(COLLECTION);
```
I think what we could change is to have a method like `cluster.getSolrClientForCollection(COLLECTION)` and it looks up a map of SolrClients by collection name, and if it has it, returns it, and if it doesn't have it, then creates it. Then in our cleanup code we shut them all down?
Alternatively, we could just create our CloudSolrClient right there, and not try to reuse them. Then we could introduce more try/with resources patterns in those tests....?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060102824
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2669,6 +2669,18 @@ public static CloudSolrClient getCloudSolrClient(String zkHost) {
.build();
}
+ /**
+ * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
+ * not wish to have any randomized behavior should use the {@link
+ * org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} class directly
+ */
+ public static CloudSolrClient getCloudSolrClient(String zkHost, String defaultCollection) {
Review Comment:
I could get behind eliminating these.... It makes SolrTestCaseJ4 super long and busy.....
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1143933928
##########
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:
I'm not sure I see the benefit of throwing an error when a different collection is used than the default. I feel like the idea behind this option is just to make it easier for people to not have to specify the collection in their commands most of the time. If you are explicitly setting a collection in a command, its pretty unlikely that that was an error...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1138692775
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java:
##########
@@ -76,7 +77,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@LuceneTestCase.Nightly
+// @LuceneTestCase.Nightly
Review Comment:
thanks... I finally learned how to run nightly on the command line, though not how to do it in intellij.....
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1367432482
@stillalex would love you eyes on this...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060203879
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -147,6 +147,8 @@ public void beforeTest() {
protected volatile ChaosMonkey chaosMonkey;
protected Map<String, CloudJettyRunner> shardToLeaderJetty = new ConcurrentHashMap<>();
+ protected volatile Map<String, CloudSolrClient> solrClientForCollectionCache =
Review Comment:
umm... i thought maybe becasue multiple things reading? i can change.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1369252897
@dsmiley there are 45 more tests that I need to fix. I wanted to just check in that the way I am migrating the tests makes sense to you?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1138691803
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZk2TestBase.java:
##########
@@ -325,7 +325,7 @@ private void bringDownShardIndexSomeDocsAndRecover() throws Exception {
query("q", "*:*", "sort", "n_tl1 desc");
- cloudClient.setDefaultCollection(DEFAULT_COLLECTION);
+ // cloudClient.withDefaultCollection(DEFAULT_COLLECTION);
Review Comment:
Thanks!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1059074849
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -331,10 +331,8 @@ protected boolean useTlogReplicas() {
}
protected CloudSolrClient createCloudClient(String defaultCollection) {
- CloudSolrClient client =
- getCloudSolrClient(zkServer.getZkAddress(), random().nextBoolean(), 30000, 120000);
- if (defaultCollection != null) client.setDefaultCollection(defaultCollection);
- return client;
+ return getCloudSolrClient(
Review Comment:
<picture><img alt="19% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/19/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.createCloudClient(...)` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=364683266&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=364683266&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=364683266&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=364683266&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=364683266&lift_comment_rating=5) ]
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060173672
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2669,6 +2669,18 @@ public static CloudSolrClient getCloudSolrClient(String zkHost) {
.build();
}
+ /**
+ * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
+ * not wish to have any randomized behavior should use the {@link
+ * org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} class directly
+ */
+ public static CloudSolrClient getCloudSolrClient(String zkHost, String defaultCollection) {
Review Comment:
My ask in this PR is only not to introduce new ones.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060214019
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -147,6 +147,8 @@ public void beforeTest() {
protected volatile ChaosMonkey chaosMonkey;
protected Map<String, CloudJettyRunner> shardToLeaderJetty = new ConcurrentHashMap<>();
+ protected volatile Map<String, CloudSolrClient> solrClientForCollectionCache =
Review Comment:
`volatile` has to do with ensuring threads can read it if it is written to by another thread. This refers to the field itself, not what may be inside (i.e. it is to the actual field reference to CHM, not what's *in* CHM). Since you set a value at the declaration and never set it anywhere else, just remove "volatile" and make it final as well to communicate this fact.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142529353
##########
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:
done
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1136279376
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2291,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
+ CloudSolrClient solrClient =
+ getCloudSolrClient(
+ zkServer.getZkAddress(), collectionName, random().nextBoolean(), 5000, 120000);
+ solrClient.connect();
+ solrClientForCollectionCache.put(collectionName, solrClient);
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Created solrClient for collection {} with updatesToLeaders={} and parallelUpdates={}",
+ collectionName,
+ solrClient.isUpdatesToLeaders(),
+ solrClient.isParallelUpdates());
+ }
+ }
+ }
+ return solrClientForCollectionCache.get(collectionName);
Review Comment:
FYI Lift->Infer->RacerD has a flaw in which it has no knowledge that ConcurrentHashMap is thread-safe.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1367439055
We have places in our tests where the code looks like:
```
indexDoc("collection2", getDoc(id, "10000000"));
indexDoc("collection2", getDoc(id, "10000001"));
indexDoc("collection2", getDoc(id, "10000003"));
getCommonCloudSolrClient().setDefaultCollection("collection2");
getCommonCloudSolrClient().add(getDoc(id, "10000004"));
getCommonCloudSolrClient().setDefaultCollection(null);
indexDoc("collection3", getDoc(id, "20000000"));
indexDoc("collection3", getDoc(id, "20000001"));
getCommonCloudSolrClient().setDefaultCollection("collection3");
```
We could just have multiple clients for each default collection? We could have some sort of map of clients by default collection so you pick the one you need.. "null", "collection2", "collection3"....
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148552968
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -182,17 +184,32 @@ public void testReplicasInLowerTerms() throws Exception {
assertEquals(
"Expected only 2 documents in the index",
2,
- cloudClient.query(params).getResults().getNumFound());
+ cloudClient.query(DEFAULT_COLLECTION, params).getResults().getNumFound());
}
- bringBackOldLeaderAndSendDoc(testCollectionName, leader, notLeaders, 5);
+ bringBackOldLeaderAndSendDoc(DEFAULT_COLLECTION, leader, notLeaders, 5);
} finally {
log.info("Cleaning up after the test.");
// try to clean up
- attemptCollectionDelete(cloudClient, testCollectionName);
+ attemptCollectionDelete(cloudClient, DEFAULT_COLLECTION);
}
}
+ /**
+ * For this test, we need a cloudClient that is not randomized since we need to NEVER send the
+ * updates only to the leader. The way the RandomizingCloudSolrClientBuilder works, you can't
+ * avoid its internal decision-making process to sometimes send updates only to leaders.
+ */
+ @Override
+ protected CloudSolrClient createCloudClient(String defaultCollection) {
+ CloudLegacySolrClient.Builder builder =
+ new CloudLegacySolrClient.Builder(
+ Collections.singletonList(zkServer.getZkAddress()), Optional.empty());
+ defaultCollection = DEFAULT_COLLECTION;
Review Comment:
okay, rewrote the docs a bit to highlight the why, and made it clearer we are only using DEFAULT_COLLECTION....
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1482888880
Okay, I think I am done! Every `CloudSolrClient.setDefaultCollection` call has been removed from the tests. I'd love some review with an eye to merging to `main` next week, and letting the tests run a few days, and then backport to `branch_9x`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1143994795
##########
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:
Right; but nearby, easy to **not** specify a collection in a command and **not** communicate with the intended collection. For example imagine some code that looks like:
```
addSomeDocs(collectionName)
....
commit()
```
My real wish/preference isn't reflected in this PR at all actually, which is for there to be a collection scoped thing that you can interact with, instead of many overloads that are optional. Maybe let's keep defaultCollection as that and then some future day work towards that (if I can convince anyone).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142530226
##########
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:
Restored back to what you had... thanks!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060127703
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2669,6 +2669,18 @@ public static CloudSolrClient getCloudSolrClient(String zkHost) {
.build();
}
+ /**
+ * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
+ * not wish to have any randomized behavior should use the {@link
+ * org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} class directly
+ */
+ public static CloudSolrClient getCloudSolrClient(String zkHost, String defaultCollection) {
Review Comment:
I opened up SOLR-16604 as a seperate JIRA to track this.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060167988
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2292,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
+ CloudSolrClient solrClient =
+ getCloudSolrClient(
+ zkServer.getZkAddress(), collectionName, random().nextBoolean(), 5000, 120000);
+ solrClient.connect();
+ solrClientForCollectionCache.put(collectionName, solrClient);
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Created solrClient for collection {} with updatesToLeaders={} and parallelUpdates={}",
+ collectionName,
+ solrClient.isUpdatesToLeaders(),
+ solrClient.isParallelUpdates());
+ }
+ }
+ }
+ return solrClientForCollectionCache.get(collectionName);
Review Comment:
<picture><img alt="19% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/19/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `AbstractFullDistribZkTestBase.getSolrClientForCollection(...)` reads without synchronization from container `this.solrClientForCollectionCache` via call to `Map.get(...)`. Potentially races with write in method `AbstractFullDistribZkTestBase.getSolrClientForCollection(...)`.
Reporting because this access may occur on a background thread.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=365179325&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=365179325&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365179325&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365179325&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=365179325&lift_comment_rating=5) ]
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -1990,13 +1990,22 @@ protected void destroyServers() throws Exception {
IOUtils.closeQuietly(c);
}));
+ customThreadPool.submit(
+ () ->
+ solrClientForCollectionCache.values().parallelStream()
+ .forEach(
+ c -> {
+ IOUtils.closeQuietly(c);
+ }));
+
customThreadPool.submit(() -> IOUtils.closeQuietly(controlClientCloud));
customThreadPool.submit(() -> IOUtils.closeQuietly(cloudClient));
ExecutorUtil.shutdownAndAwaitTermination(customThreadPool);
coreClients.clear();
+ solrClientForCollectionCache.clear();
Review Comment:
<picture><img alt="19% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/19/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `AbstractFullDistribZkTestBase.destroyServers()` mutates container `this.solrClientForCollectionCache` via call to `Map.clear()` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=365180333&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=365180333&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365180333&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365180333&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=365180333&lift_comment_rating=5) ]
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractRecoveryZkTestBase.java:
##########
@@ -81,51 +86,53 @@ public void test() throws Exception {
}
log.info("Indexing {} documents", maxDoc);
- final StoppableIndexingThread indexThread =
- new StoppableIndexingThread(null, cluster.getSolrClient(), "1", true, maxDoc, 1, true);
- threads.add(indexThread);
- indexThread.start();
+ try (SolrClient solrClient = cluster.basicSolrClientBuilder().withDefaultCollection(collection).build();) {
+ final StoppableIndexingThread indexThread =
+ new StoppableIndexingThread(null, solrClient, "1", true, maxDoc, 1, true);
+ threads.add(indexThread);
+ indexThread.start();
- final StoppableIndexingThread indexThread2 =
- new StoppableIndexingThread(null, cluster.getSolrClient(), "2", true, maxDoc, 1, true);
- threads.add(indexThread2);
- indexThread2.start();
+ final StoppableIndexingThread indexThread2 =
+ new StoppableIndexingThread(null, solrClient, "2", true, maxDoc, 1, true);
+ threads.add(indexThread2);
+ indexThread2.start();
- // give some time to index...
- int[] waitTimes = new int[] {200, 2000, 3000};
- Thread.sleep(waitTimes[random().nextInt(waitTimes.length - 1)]);
+ // give some time to index...
+ int[] waitTimes = new int[]{200, 2000, 3000};
+ Thread.sleep(waitTimes[random().nextInt(waitTimes.length - 1)]);
- // bring shard replica down
- DocCollection state = getCollectionState(collection);
- Replica leader = state.getLeader("shard1");
- Replica replica = getRandomReplica(state.getSlice("shard1"), (r) -> !leader.equals(r));
+ // bring shard replica down
+ DocCollection state = getCollectionState(collection);
+ Replica leader = state.getLeader("shard1");
+ Replica replica = getRandomReplica(state.getSlice("shard1"), (r) -> !leader.equals(r));
- JettySolrRunner jetty = cluster.getReplicaJetty(replica);
- jetty.stop();
+ JettySolrRunner jetty = cluster.getReplicaJetty(replica);
Review Comment:
<picture><img alt="17% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/17/display.svg"></picture>
*NULL_DEREFERENCE:* object `replica` last assigned on line 107 could be null and is dereferenced by call to `getReplicaJetty(...)` at line 109.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=365180147&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=365180147&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365180147&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365180147&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=365180147&lift_comment_rating=5) ]
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1470024729
I wish Crave was running these tests ;-) It's gotten in that weird state, and I'm nervous about messing with git to try and fix it...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142529060
##########
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:
So, I have gone back and forth on this. it feels weird to use a "" string as a magic meaning... aRgualby no collection should be specified by a null instead of a "". And solrClient is now actually only ever used internally to MiniSolrCloudCluster...
##########
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:
Done!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148546799
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -307,16 +340,24 @@ public void testDeleteByIdCompositeRouterWithRouterField() throws Exception {
// including cloudClient helps us test view from other nodes that aren't the
// leaders...
for (SolrClient c : Arrays.asList(cloudClient, shard1, shard2)) {
+ String queryWithCollectionName =
Review Comment:
i definilty felt icky on this one... sometimes it's a string value and sometimes it's a null.... going to look again...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060094911
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -257,6 +259,12 @@ public Builder setRetryExpiryTime(int secs) {
return this;
}
+ /** Sets the default collection for request. */
+ public Builder setDefaultCollection(String collection) {
Review Comment:
`with`
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudLegacySolrClient.java:
##########
@@ -328,6 +330,12 @@ public Builder withParallelUpdates(boolean parallelUpdates) {
return this;
}
+ /** Sets the default collection for request. */
+ public Builder setDefaultCollection(String collection) {
Review Comment:
`with`
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2669,6 +2669,18 @@ public static CloudSolrClient getCloudSolrClient(String zkHost) {
.build();
}
+ /**
+ * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
+ * not wish to have any randomized behavior should use the {@link
+ * org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} class directly
+ */
+ public static CloudSolrClient getCloudSolrClient(String zkHost, String defaultCollection) {
Review Comment:
I honestly don't like these methods. In a sense it hides a builder, -- it's as if we don't actually like builders and instead we like many overloaded methods that takes all sorts of parameters that are hard to decipher at the call site. Yuck! We should should want to embrace builders!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060204103
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -1990,13 +1990,22 @@ protected void destroyServers() throws Exception {
IOUtils.closeQuietly(c);
}));
+ customThreadPool.submit(
+ () ->
+ solrClientForCollectionCache.values().parallelStream()
Review Comment:
right above it was `coreClients.parallelStream()`, so I copied it...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060203187
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2669,6 +2669,18 @@ public static CloudSolrClient getCloudSolrClient(String zkHost) {
.build();
}
+ /**
+ * This method <i>may</i> randomize unspecified aspects of the resulting SolrClient. Tests that do
+ * not wish to have any randomized behavior should use the {@link
+ * org.apache.solr.client.solrj.impl.CloudSolrClient.Builder} class directly
+ */
+ public static CloudSolrClient getCloudSolrClient(String zkHost, String defaultCollection) {
Review Comment:
Makes sense.. I went and looked, and they all pretty much were only used by 1 test method...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1370233400
I also in #10b5061 just pushed up another idea, which is adding to the `MiniSolrCluster` a `setDefaultCollection` that would change the default `getSolrClient` ???
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1061004650
##########
solr/test-framework/src/java/org/apache/solr/cloud/MiniSolrCloudCluster.java:
##########
@@ -983,6 +1028,17 @@ public void dumpCoreInfo(PrintStream pw) throws IOException {
}
}
+ /** When this is called we recreate the SolrClient with a defaultCollection **/
+ public void setDefaultCollection(String collectionName) {
+ IOUtils.closeQuietly(solrClient);
+ solrClient = new CloudLegacySolrClient.Builder(
+ Collections.singletonList(getZkServer().getZkAddress()), Optional.empty())
+ .withSocketTimeout(90000)
+ .withConnectionTimeout(15000)
+ .withDefaultCollection(collectionName)
+ .build(); // we choose 90 because we run in some harsh envs
Review Comment:
<picture><img alt="7% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/7/display.svg"></picture>
*THREAD_SAFETY_VIOLATION:* Unprotected write. Non-private method `MiniSolrCloudCluster.setDefaultCollection(...)` indirectly mutates container `util.ObjectReleaseTracker.OBJECTS` via call to `Map.put(...)` outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=365601311&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=365601311&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365601311&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=365601311&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=365601311&lift_comment_rating=5) ]
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142531595
##########
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:
Would you like to push up an example of what you are thinking? I'm not wedded to how this works ;-).
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148385132
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -44,6 +46,7 @@
@Nightly // this test is currently too slow for non-nightly
public class ForceLeaderTest extends HttpPartitionTest {
+ public static final String DEFAULT_COLLECTION = "forceleader_lower_terms_collection";
Review Comment:
Why call this constant that? The "DEFAULT"-ness is unclear while reading the code... I thought oh this is "collection1" when really it's something test specific. I suggest simply calling this COLLECTION (sufficient) or COLLECTION_NAME (longer, I don't prefer but okay).
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -182,17 +184,32 @@ public void testReplicasInLowerTerms() throws Exception {
assertEquals(
"Expected only 2 documents in the index",
2,
- cloudClient.query(params).getResults().getNumFound());
+ cloudClient.query(DEFAULT_COLLECTION, params).getResults().getNumFound());
}
- bringBackOldLeaderAndSendDoc(testCollectionName, leader, notLeaders, 5);
+ bringBackOldLeaderAndSendDoc(DEFAULT_COLLECTION, leader, notLeaders, 5);
} finally {
log.info("Cleaning up after the test.");
// try to clean up
- attemptCollectionDelete(cloudClient, testCollectionName);
+ attemptCollectionDelete(cloudClient, DEFAULT_COLLECTION);
}
}
+ /**
+ * For this test, we need a cloudClient that is not randomized since we need to NEVER send the
+ * updates only to the leader. The way the RandomizingCloudSolrClientBuilder works, you can't
+ * avoid its internal decision-making process to sometimes send updates only to leaders.
+ */
+ @Override
+ protected CloudSolrClient createCloudClient(String defaultCollection) {
+ CloudLegacySolrClient.Builder builder =
+ new CloudLegacySolrClient.Builder(
+ Collections.singletonList(zkServer.getZkAddress()), Optional.empty());
+ defaultCollection = DEFAULT_COLLECTION;
Review Comment:
This is highly suspicious; you are ignoring the param. Then don't pass a param. Or assert it's equivalency to this constant if you must.
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -182,17 +184,32 @@ public void testReplicasInLowerTerms() throws Exception {
assertEquals(
"Expected only 2 documents in the index",
2,
- cloudClient.query(params).getResults().getNumFound());
+ cloudClient.query(DEFAULT_COLLECTION, params).getResults().getNumFound());
}
- bringBackOldLeaderAndSendDoc(testCollectionName, leader, notLeaders, 5);
+ bringBackOldLeaderAndSendDoc(DEFAULT_COLLECTION, leader, notLeaders, 5);
} finally {
log.info("Cleaning up after the test.");
// try to clean up
- attemptCollectionDelete(cloudClient, testCollectionName);
+ attemptCollectionDelete(cloudClient, DEFAULT_COLLECTION);
}
}
+ /**
+ * For this test, we need a cloudClient that is not randomized since we need to NEVER send the
+ * updates only to the leader. The way the RandomizingCloudSolrClientBuilder works, you can't
+ * avoid its internal decision-making process to sometimes send updates only to leaders.
+ */
+ @Override
+ protected CloudSolrClient createCloudClient(String defaultCollection) {
+ CloudLegacySolrClient.Builder builder =
Review Comment:
"var" please; long name.
Or better yet, don't even store in a var; just do one long line ending in .build() that you return directly!
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -92,73 +89,86 @@ public static String createAndSetNewDefaultCollection() throws Exception {
DEFAULT_TIMEOUT,
TimeUnit.SECONDS,
(n, c) -> DocCollection.isFullyActive(n, c, 2, 2));
- cloudClient.setDefaultCollection(name);
return name;
}
@Test
public void testBasicUpdates() throws Exception {
Review Comment:
You had stated that my `withCollection` idea wasn't going to be useful for you but I think here it would have allowed you to keep this method as it was, albeit after you use withCollection in the beginning. Specifying collectionName every friggin time is tiresome (I suspect you agree).
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZkTestBase.java:
##########
@@ -1589,22 +1589,20 @@ private void testMultipleCollections() throws Exception {
indexDoc("collection2", getDoc(id, "10000000"));
indexDoc("collection2", getDoc(id, "10000001"));
indexDoc("collection2", getDoc(id, "10000003"));
- getCommonCloudSolrClient().setDefaultCollection("collection2");
- getCommonCloudSolrClient().add(getDoc(id, "10000004"));
- getCommonCloudSolrClient().setDefaultCollection(null);
+
+ getSolrClient("collection2").add(getDoc(id, "10000004"));
Review Comment:
BTW just want to thank you again for cleaning up crap like this
##########
solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java:
##########
@@ -947,9 +947,14 @@ private void searchSeveralWays(
responseConsumer.accept(cluster.getSolrClient().query(collectionList, solrQuery));
} else {
// new CloudSolrClient (random shardLeadersOnly)
- try (CloudSolrClient solrClient = getCloudSolrClient(cluster)) {
- if (random().nextBoolean()) {
- solrClient.setDefaultCollection(collectionList);
+
+ RandomizingCloudSolrClientBuilder builder = new RandomizingCloudSolrClientBuilder(cluster);
Review Comment:
"var" please; wow that type is long!
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -258,25 +278,38 @@ public void testDeleteByIdImplicitRouter() throws Exception {
}
}
+ private String specifyCollectionParameterForCloudSolrClient(
+ SolrClient solrClient, String collectionName) {
+ // For CloudSolrClient we must specify the collection name as a parameter,
+ // however for shard1 and shard2, it's already part of the URL so we can specify a null
+ // parameter. The
+ // instanceof check makes that decision.
+ String collectionNameRequiredParameter = null;
+ if (solrClient instanceof CloudSolrClient) {
+ collectionNameRequiredParameter = collectionName;
+ }
+ return collectionNameRequiredParameter;
Review Comment:
could be ternary operator one-liner and be just as clear. One liners are nice, not just for their brevity, but it also often means not naming yet another variable (and wow this one is a doozy)
##########
solr/core/src/test/org/apache/solr/cloud/ForceLeaderTest.java:
##########
@@ -284,29 +301,38 @@ private void bringBackOldLeaderAndSendDoc(
int numActiveReplicas = getNumberOfActiveReplicas(clusterState, collection, SHARD1);
assertEquals(1 + notLeaders.size(), numActiveReplicas);
log.info("Sending doc {}...", docid);
- sendDoc(docid);
+ sendDoc(collection, docid);
log.info("Committing...");
- cloudClient.commit();
+ cloudClient.commit(collection);
log.info("Doc {} sent and commit issued", docid);
assertDocsExistInAllReplicas(notLeaders, collection, docid, docid);
assertDocsExistInAllReplicas(Collections.singletonList(leader), collection, docid, docid);
}
@Override
- protected int sendDoc(int docId) throws Exception {
+ protected int sendDoc(String collectionName, int docId) throws Exception {
SolrInputDocument doc = new SolrInputDocument();
doc.addField(id, String.valueOf(docId));
doc.addField("a_t", "hello" + docId);
- return sendDocsWithRetry(Collections.singletonList(doc), 1, 5, 1);
+ return sendDocsWithRetry(collectionName, Collections.singletonList(doc), 1, 5, 1);
}
private void doForceLeader(String collectionName, String shard)
throws IOException, SolrServerException {
CollectionAdminRequest.ForceLeader forceLeader =
CollectionAdminRequest.forceLeaderElection(collectionName, shard);
+ boolean shardLeadersOnly = random().nextBoolean();
+ var builder =
+ new RandomizingCloudSolrClientBuilder(
+ Collections.singletonList(zkServer.getZkAddress()), Optional.empty());
+ if (shardLeadersOnly) {
+ builder.sendUpdatesOnlyToShardLeaders();
+ } else {
+ builder.sendUpdatesToAllReplicasInShard();
+ }
Review Comment:
I recommend removing this random choice of who you talk to. I understand what this test is testing; randomizing this particular aspect is pointless.
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -307,16 +340,24 @@ public void testDeleteByIdCompositeRouterWithRouterField() throws Exception {
// including cloudClient helps us test view from other nodes that aren't the
// leaders...
for (SolrClient c : Arrays.asList(cloudClient, shard1, shard2)) {
+ String queryWithCollectionName =
Review Comment:
Is the "queryWith" part really helpful?
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -258,25 +278,38 @@ public void testDeleteByIdImplicitRouter() throws Exception {
}
}
+ private String specifyCollectionParameterForCloudSolrClient(
+ SolrClient solrClient, String collectionName) {
+ // For CloudSolrClient we must specify the collection name as a parameter,
+ // however for shard1 and shard2, it's already part of the URL so we can specify a null
Review Comment:
I hope we can get to the point that we never ever more bake in the collection/core name into the URL of the constructor of a Http client. Not today but someday.
##########
solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java:
##########
@@ -563,25 +541,29 @@ protected void assertDocsExistInAllReplicas(
}
}
- protected SolrClient getHttpSolrClient(Replica replica, String coll) {
+ protected SolrClient getHttpSolrClient(Replica replica, String collection) {
Review Comment:
Not a fan of brevity, I see ;-). LOL
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1147967826
##########
solr/core/src/test/org/apache/solr/cloud/HttpPartitionTest.java:
##########
@@ -70,7 +65,7 @@
* Simulates HTTP partitions between a leader and replica but the replica does not lose its
* ZooKeeper connection.
*/
-@LuceneTestCase.Nightly // there are recovery commands that take a while to time out
+// @LuceneTestCase.Nightly // there are recovery commands that take a while to time out
Review Comment:
[Q] Is the commenting-out of the Nightly tag here temporary? Did you mean to remove this?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -813,6 +818,11 @@ public NamedList<Object> request(SolrRequest<?> request, String collection)
} else if (collection == null) {
collection = defaultCollection;
}
+ // TODO if we like this, then "defaultCollection" should be renamed
Review Comment:
[0] IMO this would be a bit inflexible, at least for a ticket that only set out to remove a setter.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148551005
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -258,25 +278,38 @@ public void testDeleteByIdImplicitRouter() throws Exception {
}
}
+ private String specifyCollectionParameterForCloudSolrClient(
+ SolrClient solrClient, String collectionName) {
+ // For CloudSolrClient we must specify the collection name as a parameter,
+ // however for shard1 and shard2, it's already part of the URL so we can specify a null
+ // parameter. The
+ // instanceof check makes that decision.
+ String collectionNameRequiredParameter = null;
+ if (solrClient instanceof CloudSolrClient) {
+ collectionNameRequiredParameter = collectionName;
+ }
+ return collectionNameRequiredParameter;
Review Comment:
ended up removing this method... Yeah, it was icky.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1148550914
##########
solr/core/src/test/org/apache/solr/cloud/FullSolrCloudDistribCmdsTest.java:
##########
@@ -307,16 +340,24 @@ public void testDeleteByIdCompositeRouterWithRouterField() throws Exception {
// including cloudClient helps us test view from other nodes that aren't the
// leaders...
for (SolrClient c : Arrays.asList(cloudClient, shard1, shard2)) {
+ String queryWithCollectionName =
Review Comment:
Reworked it, passing in conditionally the `collection` param as part of the `params` object... I think this is cleaner, less indirection ;-)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1137082274
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2291,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
+ CloudSolrClient solrClient =
+ getCloudSolrClient(
+ zkServer.getZkAddress(), collectionName, random().nextBoolean(), 5000, 120000);
+ solrClient.connect();
+ solrClientForCollectionCache.put(collectionName, solrClient);
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Created solrClient for collection {} with updatesToLeaders={} and parallelUpdates={}",
+ collectionName,
+ solrClient.isUpdatesToLeaders(),
+ solrClient.isParallelUpdates());
+ }
+ }
+ }
+ return solrClientForCollectionCache.get(collectionName);
Review Comment:
thanks...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1145394388
##########
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:
i think the whole `collectionName` capablity is maybe a bad idea.... It makes the CloudSolrClient and regular old SolrClient very different behaviors...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142529996
##########
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:
Yeah, as in, it didn't need to be done! Thanks for the reminder! The code change was actually just typo fixes.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1142530690
##########
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:
I wish we had a commumnity standard for when to use a var... or that `gw tidy` did it for me.. I went ahead and put one in.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060893846
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2292,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
Review Comment:
thanks for explaining, and I just pushed up the change.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #1256:
URL: https://github.com/apache/solr/pull/1256#issuecomment-1370207339
Okay, at this point, a bit stuck. I thought, hey, maybe I should introduce some sort of `getSolrClientForCollection(String collectionName)`, and so I did in #e8dfd99e6c2d3878cc6517c111d9828270973bdd, however if you look at it, it's kind of silly. at taht point I might as well just use the method `getSolrClient().add(collectionName,docs)` instead!
I *could* just update the `cluster.solrClient` to be the one created after creating the collection? and toss the old one?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060175270
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -965,11 +964,12 @@ public void testRetryUpdatesWhenClusterStateIsStale() throws Exception {
Collections.singletonList(cluster.getZkServer().getZkAddress()), Optional.empty())
.withParallelUpdates(true)
.sendDirectUpdatesToAnyShardReplica()
+ .withDefaultCollection(COL)
// don't let collection cache entries get expired, even on a slow machine...
.withCollectionCacheTtl(Integer.MAX_VALUE)
.build()) {
-
- stale_client.setDefaultCollection(COL);
+ // don't let collection cache entries get expired, even on a slow machine...
+ stale_client.setCollectionCacheTTl(Integer.MAX_VALUE);
Review Comment:
should use builder? it wasn't here before; what's the story?
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -147,6 +147,8 @@ public void beforeTest() {
protected volatile ChaosMonkey chaosMonkey;
protected Map<String, CloudJettyRunner> shardToLeaderJetty = new ConcurrentHashMap<>();
+ protected volatile Map<String, CloudSolrClient> solrClientForCollectionCache =
Review Comment:
why volatile?
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -1990,13 +1990,22 @@ protected void destroyServers() throws Exception {
IOUtils.closeQuietly(c);
}));
+ customThreadPool.submit(
+ () ->
+ solrClientForCollectionCache.values().parallelStream()
Review Comment:
parallelStream seems like overkill for a small collection of things that should close quickly. Were you inspired by something?
##########
solr/modules/hdfs/src/test/org/apache/solr/hdfs/cloud/HdfsRecoveryZkTest.java:
##########
@@ -29,7 +28,6 @@
import org.junit.BeforeClass;
@Nightly
-@LuceneTestCase.AwaitsFix(bugUrl = "SOLR-15405")
Review Comment:
very out of scope of this PR; remove
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -1990,13 +1990,22 @@ protected void destroyServers() throws Exception {
IOUtils.closeQuietly(c);
}));
+ customThreadPool.submit(
+ () ->
+ solrClientForCollectionCache.values().parallelStream()
+ .forEach(
+ c -> {
+ IOUtils.closeQuietly(c);
+ }));
+
customThreadPool.submit(() -> IOUtils.closeQuietly(controlClientCloud));
customThreadPool.submit(() -> IOUtils.closeQuietly(cloudClient));
ExecutorUtil.shutdownAndAwaitTermination(customThreadPool);
coreClients.clear();
+ solrClientForCollectionCache.clear();
Review Comment:
Seems to be a deficiency of "RacerD" inside https://github.com/facebook/infer which Lift uses.
@sonatype-lift exclude issue
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060204376
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -965,11 +964,12 @@ public void testRetryUpdatesWhenClusterStateIsStale() throws Exception {
Collections.singletonList(cluster.getZkServer().getZkAddress()), Optional.empty())
.withParallelUpdates(true)
.sendDirectUpdatesToAnyShardReplica()
+ .withDefaultCollection(COL)
// don't let collection cache entries get expired, even on a slow machine...
.withCollectionCacheTtl(Integer.MAX_VALUE)
.build()) {
-
- stale_client.setDefaultCollection(COL);
+ // don't let collection cache entries get expired, even on a slow machine...
+ stale_client.setCollectionCacheTTl(Integer.MAX_VALUE);
Review Comment:
argh, merge issue
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060216195
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2292,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
Review Comment:
No, use `computeIfAbsent` instead of synchronization, since you chose to use ConcurrentHashMap (which makes sense). It's faster and safer. This may explain why Lift/Infer complained because you are using synchronized here but not in destroyServers when you called clear() on the map.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060204038
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -1990,13 +1990,22 @@ protected void destroyServers() throws Exception {
IOUtils.closeQuietly(c);
}));
+ customThreadPool.submit(
+ () ->
+ solrClientForCollectionCache.values().parallelStream()
Review Comment:
there was another one that had it like that... and so I was mimicing it... it doesn't need to be.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1060203789
##########
solr/modules/hdfs/src/test/org/apache/solr/hdfs/cloud/HdfsRecoveryZkTest.java:
##########
@@ -29,7 +28,6 @@
import org.junit.BeforeClass;
@Nightly
-@LuceneTestCase.AwaitsFix(bugUrl = "SOLR-15405")
Review Comment:
done
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1137365537
##########
solr/core/src/test/org/apache/solr/cloud/MigrateRouteKeyTest.java:
##########
@@ -105,7 +105,6 @@ public void multipleShardMigrateTest() throws Exception {
CollectionAdminRequest.createCollection("sourceCollection", "conf", 2, 1)
Review Comment:
`"sourceCollection"` should be a variable just like targetcollection
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/ShardSplitTest.java:
##########
@@ -76,7 +77,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@LuceneTestCase.Nightly
+// @LuceneTestCase.Nightly
Review Comment:
Hm?
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2292,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
Review Comment:
Did you push up a change @epugh? Still see synchronized
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractBasicDistributedZk2TestBase.java:
##########
@@ -325,7 +325,7 @@ private void bringDownShardIndexSomeDocsAndRecover() throws Exception {
query("q", "*:*", "sort", "n_tl1 desc");
- cloudClient.setDefaultCollection(DEFAULT_COLLECTION);
+ // cloudClient.withDefaultCollection(DEFAULT_COLLECTION);
Review Comment:
remove?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1256: SOLR-10466: setDefaultCollection should be deprecated in favor of SolrClientBuilder methods
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1256:
URL: https://github.com/apache/solr/pull/1256#discussion_r1135450119
##########
solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java:
##########
@@ -2283,6 +2291,26 @@ protected CloudSolrClient getCommonCloudSolrClient() {
return commonCloudSolrClient;
}
+ protected CloudSolrClient getSolrClientForCollection(String collectionName) {
+ synchronized (this) {
+ if (!solrClientForCollectionCache.containsKey(collectionName)) {
+ CloudSolrClient solrClient =
+ getCloudSolrClient(
+ zkServer.getZkAddress(), collectionName, random().nextBoolean(), 5000, 120000);
+ solrClient.connect();
+ solrClientForCollectionCache.put(collectionName, solrClient);
+ if (log.isInfoEnabled()) {
+ log.info(
+ "Created solrClient for collection {} with updatesToLeaders={} and parallelUpdates={}",
+ collectionName,
+ solrClient.isUpdatesToLeaders(),
+ solrClient.isParallelUpdates());
+ }
+ }
+ }
+ return solrClientForCollectionCache.get(collectionName);
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Read/Write race. Non-private method `AbstractFullDistribZkTestBase.getSolrClientForCollection(...)` reads without synchronization from container `this.solrClientForCollectionCache` via call to `Map.get(...)`. Potentially races with write in method `AbstractFullDistribZkTestBase.destroyServers()`.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=433680238&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=433680238&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=433680238&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=433680238&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=433680238&lift_comment_rating=5)â]
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org