epugh commented on code in PR #2231:
URL: https://github.com/apache/solr/pull/2231#discussion_r1471057983
##########
solr/core/src/test/org/apache/solr/handler/ReplicationTestHelper.java:
##########
@@ -64,8 +64,20 @@ public static JettySolrRunner
createAndStartJetty(SolrInstance instance) throws
return jetty;
}
+ /**
+ * @param baseUrl the root URL for a Solr node
+ */
public static SolrClient createNewSolrClient(String baseUrl) {
+ return createNewSolrClient(baseUrl, null);
+ }
+
+ /**
+ * @param baseUrl the root URL for a Solr node
+ * @param collectionOrCore an optional default collection/core for the
created client
+ */
+ public static SolrClient createNewSolrClient(String baseUrl, String
collectionOrCore) {
Review Comment:
somewhat surprises this method doesn't exist on a parent class!
##########
solr/core/src/test/org/apache/solr/handler/admin/CoreAdminHandlerTest.java:
##########
@@ -377,7 +378,8 @@ public void testUnloadForever() throws Exception {
runner.start();
try (SolrClient client =
- new HttpSolrClient.Builder(runner.getBaseUrl() + "/corex")
+ new HttpSolrClient.Builder(runner.getBaseUrl().toString())
Review Comment:
i see if everywhere now that we are losing the type coercion by appending
the collection or core name!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java:
##########
@@ -100,8 +101,13 @@ public class ConcurrentUpdateSolrClient extends SolrClient
{
*/
protected ConcurrentUpdateSolrClient(Builder builder) {
this.internalHttpClient = (builder.httpClient == null);
+ final var hscBuilder =
Review Comment:
might just be me, but `hscBuilder` didn't really ring for me... `builder`?
`httpSolrClientBuilder` ? I don't love the initials style...
##########
solr/core/src/test/org/apache/solr/handler/admin/CoreAdminHandlerTest.java:
##########
@@ -377,7 +378,8 @@ public void testUnloadForever() throws Exception {
runner.start();
try (SolrClient client =
- new HttpSolrClient.Builder(runner.getBaseUrl() + "/corex")
+ new HttpSolrClient.Builder(runner.getBaseUrl().toString())
Review Comment:
i wonder if the Builder should just take a Url? Does we end up doing a
.toString a million times?
##########
solr/core/src/test/org/apache/solr/cloud/LeaderTragicEventTest.java:
##########
@@ -128,9 +131,9 @@ private Replica corruptLeader(String collection) throws
IOException, SolrServerE
Replica oldLeader = dc.getLeader("shard1");
log.info("Will crash leader : {}", oldLeader);
- try (SolrClient solrClient =
- new
HttpSolrClient.Builder(dc.getLeader("shard1").getCoreUrl()).build()) {
- new UpdateRequest().add("id", "99").commit(solrClient, null);
+ final Replica leaderReplica = dc.getLeader("shard1");
+ try (SolrClient solrClient = new
HttpSolrClient.Builder(leaderReplica.getBaseUrl()).build()) {
+ new UpdateRequest().add("id", "99").commit(solrClient,
leaderReplica.getCoreName());
Review Comment:
i like not seeing a `null` passed in!
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -144,6 +145,14 @@ private static CloudHttp2SolrClient
newCloudHttp2SolrClient(
return client;
}
+ /**
+ * Create (and cache) a SolrClient based around the provided URL
Review Comment:
nice! some docs!
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -161,8 +170,12 @@ public synchronized SolrClient getHttpSolrClient(String
baseUrl) {
}
@Deprecated
- private static SolrClient newHttpSolrClient(String baseUrl, HttpClient
httpClient) {
- HttpSolrClient.Builder builder = new HttpSolrClient.Builder(baseUrl);
+ private static SolrClient newHttpSolrClient(String url, HttpClient
httpClient) {
Review Comment:
this must have been some fun futzy logic.. comes out looking nice.
##########
solr/core/src/test/org/apache/solr/client/solrj/impl/ConnectionReuseTest.java:
##########
@@ -89,7 +89,8 @@ private SolrClient buildClient(CloseableHttpClient
httpClient, URL url) {
.withThreadCount(1)
.build();
case 1:
- return new HttpSolrClient.Builder(url.toString() + "/" + COLLECTION)
+ return new HttpSolrClient.Builder(url.toString())
Review Comment:
this is nicer! less magic string manipulation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
