dsmiley commented on code in PR #2899:
URL: https://github.com/apache/solr/pull/2899#discussion_r1887778948
##########
solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java:
##########
@@ -37,14 +36,15 @@ final class HttpSolrClientProvider implements AutoCloseable
{
private final Http2SolrClient httpSolrClient;
+ private final Http2SolrClient.Builder httpClientBuilder;
+
private final InstrumentedHttpListenerFactory trackHttpSolrMetrics;
HttpSolrClientProvider(UpdateShardHandlerConfig cfg, SolrMetricsContext
parentContext) {
trackHttpSolrMetrics = new
InstrumentedHttpListenerFactory(getNameStrategy(cfg));
initializeMetrics(parentContext);
- Http2SolrClient.Builder httpClientBuilder =
- new
Http2SolrClient.Builder().withListenerFactory(List.of(trackHttpSolrMetrics));
+ this.httpClientBuilder = new
Http2SolrClient.Builder().addListenerFactory(trackHttpSolrMetrics);
Review Comment:
name httpSolrClientBuilder or similar
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
@@ -872,15 +868,14 @@ public SolrCloudManager getSolrCloudManager() {
if (cloudManager != null) {
return cloudManager;
}
- http2SolrClient =
+ var httpClientBuilder =
Review Comment:
again, should have a name like "solrClientBuilder" (maybe adding "http") to
clarify this is building a SolrClient not an HttpClient.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -103,15 +104,14 @@ public void testSynchronousWithFalures() throws Exception
{
LBSolrClient.Endpoint ep2 = new
LBSolrClient.Endpoint("http://endpoint.two";);
List<LBSolrClient.Endpoint> endpointList = List.of(ep1, ep2);
- Http2SolrClient.Builder b =
- new
Http2SolrClient.Builder("http://base.url";).withConnectionTimeout(10,
TimeUnit.SECONDS);
- ;
- try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url";,
b);
- LBHttp2SolrClient<MockHttpSolrClient> testClient =
- new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+ var httpClientBuilder =
Review Comment:
httpSolrClientBuilder
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -74,12 +76,11 @@ public void testSynchronous() throws Exception {
LBSolrClient.Endpoint ep2 = new
LBSolrClient.Endpoint("http://endpoint.two";);
List<LBSolrClient.Endpoint> endpointList = List.of(ep1, ep2);
- Http2SolrClient.Builder b =
- new
Http2SolrClient.Builder("http://base.url";).withConnectionTimeout(10,
TimeUnit.SECONDS);
- ;
- try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url";,
b);
- LBHttp2SolrClient<MockHttpSolrClient> testClient =
- new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+ var httpClientBuilder =
Review Comment:
httpSolrClientBuilder
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -404,23 +400,6 @@ public Builder withCollectionCacheTtl(long timeToLive,
TimeUnit unit) {
return this;
}
- /**
- * Set the internal http client.
- *
- * <p>Note: closing the httpClient instance is at the responsibility of
the caller.
- *
- * @param httpClient http client
- * @return this
- */
- public Builder withHttpClient(Http2SolrClient httpClient) {
Review Comment:
not sure we want to take this away from users
##########
solr/core/src/java/org/apache/solr/security/HttpClientBuilderPlugin.java:
##########
@@ -34,4 +34,8 @@ public interface HttpClientBuilderPlugin {
public SolrHttpClientBuilder getHttpClientBuilder(SolrHttpClientBuilder
builder);
public default void setup(Http2SolrClient client) {}
+
+ public default void setup(Http2SolrClient.Builder httpClientBuilder,
Http2SolrClient client) {
Review Comment:
Maybe these should be split up; setup a client and builder separately (2
methods)? Feels weird to take both in setup().
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -51,11 +52,12 @@ public void
testLBHttp2SolrClientWithTheseParamNamesInTheUrl() {
Set<String> urlParamNames = new HashSet<>(2);
urlParamNames.add("param1");
- try (Http2SolrClient http2SolrClient =
- new
Http2SolrClient.Builder(url).withTheseParamNamesInTheUrl(urlParamNames).build();
- LBHttp2SolrClient<Http2SolrClient> testClient =
- new LBHttp2SolrClient.Builder<>(http2SolrClient, new
LBSolrClient.Endpoint(url))
- .build()) {
+ var httpClientBuilder =
Review Comment:
httpSolrClientBuilder
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -94,35 +97,63 @@
*
* @since solr 8.0
*/
-public class LBHttp2SolrClient<C extends HttpSolrClientBase> extends
LBSolrClient {
+public class LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>>
extends LBSolrClient {
Review Comment:
an aside -- I wonder if the use of generics here is more annoying than
helpful. We aren't compelled to use generics.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -162,28 +154,35 @@ public void testAsyncWithFailures() {
LBSolrClient.Endpoint ep2 = new
LBSolrClient.Endpoint("http://endpoint.two";);
List<LBSolrClient.Endpoint> endpointList = List.of(ep1, ep2);
- Http2SolrClient.Builder b =
- new
Http2SolrClient.Builder("http://base.url";).withConnectionTimeout(10,
TimeUnit.SECONDS);
- ;
- try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url";,
b);
- LBHttp2SolrClient<MockHttpSolrClient> testClient =
- new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+ var httpClientBuilder =
Review Comment:
httpSolrClientBuilder
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudHttp2SolrClient.java:
##########
@@ -54,8 +53,8 @@ public class CloudHttp2SolrClient extends CloudSolrClient {
*/
protected CloudHttp2SolrClient(Builder builder) {
super(builder.shardLeadersOnly, builder.parallelUpdates,
builder.directUpdatesToLeadersOnly);
- this.clientIsInternal = builder.httpClient == null;
- this.myClient = createOrGetHttpClientFromBuilder(builder);
+ var httpClientBuilder = createOrGetHttpClientBuilder(builder);
Review Comment:
httpSolrClientBuilder
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -932,8 +931,27 @@ public Builder(String baseSolrUrl) {
this.baseSolrUrl = baseSolrUrl;
}
- public Http2SolrClient.Builder
withListenerFactory(List<HttpListenerFactory> listenerFactory) {
- this.listenerFactory = listenerFactory;
+ /**
+ * specify a listener factory, which will be appened to any existing
values.
Review Comment:
```suggestion
* Specify a listener factory, which will be appended to any existing
values.
```
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientTest.java:
##########
@@ -227,11 +226,11 @@ public void testAsync() {
LBSolrClient.Endpoint ep2 = new
LBSolrClient.Endpoint("http://endpoint.two";);
List<LBSolrClient.Endpoint> endpointList = List.of(ep1, ep2);
- Http2SolrClient.Builder b =
- new
Http2SolrClient.Builder("http://base.url";).withConnectionTimeout(10,
TimeUnit.SECONDS);
- try (MockHttpSolrClient client = new MockHttpSolrClient("http://base.url";,
b);
- LBHttp2SolrClient<MockHttpSolrClient> testClient =
- new LBHttp2SolrClient.Builder<>(client, ep1, ep2).build()) {
+ var httpClientBuilder =
Review Comment:
httpSolrClientBuilder
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
