diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 85535c3a668..9e9b1172064 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -29,7 +29,7 @@ Upgrading from Solr 6.x ./server/scripts/cloud-scripts/zkcli.sh -zkhost 127.0.0.1:2181 -cmd clusterprop -name legacyCloud -val true -* HttpClientInterceptorPlugin is now HttpClientBuilderPlugin and must work with a +* HttpClientInterceptorPlugin is now HttpClientBuilderPlugin and must work with a SolrHttpClientBuilder rather than an HttpClientConfigurer. * HttpClientUtil now allows configuring HttpClient instances via SolrHttpClientBuilder @@ -318,6 +318,10 @@ Other Changes resources, they are only resolved against Solr's own or "core" class loader by default. (Uwe Schindler) +* SOLR-10915: Make builder based SolrClient constructors to be the only valid way to construct client objects and + increase the visibility of builder elements to be protected so extending the builder, and the clients is possible. + (Jason Gerlowski, Anshum Gupta) + ================== 6.7.0 ================== Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release. diff --git a/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java b/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java index 7c630f42453..316aa455a1f 100644 --- a/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java +++ b/solr/core/src/java/org/apache/solr/update/StreamingSolrClients.java @@ -72,7 +72,13 @@ public class StreamingSolrClients { // NOTE: increasing to more than 1 threadCount for the client could cause updates to be reordered // on a greater scale since the current behavior is to only increase the number of connections/Runners when // the queue is more than half full. - client = new ErrorReportingConcurrentUpdateSolrClient(url, httpClient, 100, runnerCount, updateExecutor, true, req); + client = new ErrorReportingConcurrentUpdateSolrClient.Builder(url, req, errors) + .withHttpClient(httpClient) + .withQueueSize(100) + .withThreadCount(runnerCount) + .withExecutorService(updateExecutor) + .alwaysStreamDeletes() + .build(); client.setPollQueueTime(Integer.MAX_VALUE); // minimize connections created client.setParser(new BinaryResponseParser()); client.setRequestWriter(new BinaryRequestWriter()); @@ -115,31 +121,48 @@ public class StreamingSolrClients { public ExecutorService getUpdateExecutor() { return updateExecutor; } +} + +class ErrorReportingConcurrentUpdateSolrClient extends ConcurrentUpdateSolrClient { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + private final SolrCmdDistributor.Req req; + private final List errors; - class ErrorReportingConcurrentUpdateSolrClient extends ConcurrentUpdateSolrClient { - private final SolrCmdDistributor.Req req; + public ErrorReportingConcurrentUpdateSolrClient(Builder builder) { + super(builder); + this.req = builder.req; + this.errors = builder.errors; + } + + @Override + public void handleError(Throwable ex) { + req.trackRequestResult(null, false); + log.error("error", ex); + Error error = new Error(); + error.e = (Exception) ex; + if (ex instanceof SolrException) { + error.statusCode = ((SolrException) ex).code(); + } + error.req = req; + errors.add(error); + } + @Override + public void onSuccess(HttpResponse resp) { + req.trackRequestResult(resp, true); + } + + static class Builder extends ConcurrentUpdateSolrClient.Builder { + protected SolrCmdDistributor.Req req; + protected List errors; - public ErrorReportingConcurrentUpdateSolrClient(String solrServerUrl, HttpClient client, int queueSize, - int threadCount, ExecutorService es, boolean streamDeletes, SolrCmdDistributor.Req req) { - super(solrServerUrl, client, queueSize, threadCount, es, streamDeletes); + public Builder(String baseSolrUrl, SolrCmdDistributor.Req req, List errors) { + super(baseSolrUrl); this.req = req; + this.errors = errors; } - @Override - public void handleError(Throwable ex) { - req.trackRequestResult(null, false); - log.error("error", ex); - Error error = new Error(); - error.e = (Exception) ex; - if (ex instanceof SolrException) { - error.statusCode = ((SolrException) ex).code(); - } - error.req = req; - errors.add(error); - } - @Override - public void onSuccess(HttpResponse resp) { - req.trackRequestResult(resp, true); + public ErrorReportingConcurrentUpdateSolrClient build() { + return new ErrorReportingConcurrentUpdateSolrClient(this); } } } diff --git a/solr/core/src/test/org/apache/solr/cloud/FullThrottleStoppableIndexingThread.java b/solr/core/src/test/org/apache/solr/cloud/FullThrottleStoppableIndexingThread.java index 909f39afab2..c33ad4f05ee 100644 --- a/solr/core/src/test/org/apache/solr/cloud/FullThrottleStoppableIndexingThread.java +++ b/solr/core/src/test/org/apache/solr/cloud/FullThrottleStoppableIndexingThread.java @@ -22,7 +22,6 @@ import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; -import org.apache.http.client.HttpClient; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.lucene.util.LuceneTestCase; import org.apache.solr.client.solrj.SolrClient; @@ -55,7 +54,11 @@ class FullThrottleStoppableIndexingThread extends StoppableIndexingThread { setDaemon(true); this.clients = clients; - cusc = new ErrorLoggingConcurrentUpdateSolrClient(((HttpSolrClient) clients.get(0)).getBaseURL(), httpClient, 8, 2); + cusc = new ErrorLoggingConcurrentUpdateSolrClient.Builder(((HttpSolrClient) clients.get(0)).getBaseURL()) + .withHttpClient(httpClient) + .withQueueSize(8) + .withThreadCount(2) + .build(); cusc.setConnectionTimeout(10000); cusc.setSoTimeout(clientSoTimeout); } @@ -114,8 +117,11 @@ class FullThrottleStoppableIndexingThread extends StoppableIndexingThread { clientIndex = 0; } cusc.shutdownNow(); - cusc = new ErrorLoggingConcurrentUpdateSolrClient(((HttpSolrClient) clients.get(clientIndex)).getBaseURL(), - httpClient, 30, 3); + cusc = new ErrorLoggingConcurrentUpdateSolrClient.Builder(((HttpSolrClient) clients.get(clientIndex)).getBaseURL()) + .withHttpClient(httpClient) + .withQueueSize(30) + .withThreadCount(3) + .build(); } } @@ -143,14 +149,25 @@ class FullThrottleStoppableIndexingThread extends StoppableIndexingThread { } static class ErrorLoggingConcurrentUpdateSolrClient extends ConcurrentUpdateSolrClient { - @SuppressWarnings("deprecation") - public ErrorLoggingConcurrentUpdateSolrClient(String serverUrl, HttpClient httpClient, int queueSize, int threadCount) { - super(serverUrl, httpClient, queueSize, threadCount, null, false); + public ErrorLoggingConcurrentUpdateSolrClient(Builder builder) { + super(builder); } + @Override public void handleError(Throwable ex) { log.warn("cusc error", ex); } + + static class Builder extends ConcurrentUpdateSolrClient.Builder { + + public Builder(String baseSolrUrl) { + super(baseSolrUrl); + } + + public ErrorLoggingConcurrentUpdateSolrClient build() { + return new ErrorLoggingConcurrentUpdateSolrClient(this); + } + } } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java index f0684f85f21..6cd5caffe1d 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java @@ -233,77 +233,44 @@ public class CloudSolrClient extends SolrClient { retriedAt = System.nanoTime(); } } - + /** * Create a new client object that connects to Zookeeper and is always aware * of the SolrCloud state. If there is a fully redundant Zookeeper quorum and * SolrCloud has enough replicas for every shard in a collection, there is no * single point of failure. Updates will be sent to shard leaders by default. * - * @param zkHosts - * A Java Collection (List, Set, etc) of HOST:PORT strings, one for - * each host in the zookeeper ensemble. Note that with certain - * Collection types like HashSet, the order of hosts in the final - * connect string may not be in the same order you added them. - * Provide only one of solrUrls or zkHosts. - * @param chroot - * A chroot value for zookeeper, starting with a forward slash. If no - * chroot is required, use null. - * @param solrUrls - * A list of Solr URLs to configure the underlying {@link HttpClusterStateProvider}, which will - * use of the these URLs to fetch the list of live nodes for this Solr cluster. URL's must point to the - * root Solr path ("/solr"). Provide only one of solrUrls or zkHosts. - * @param httpClient - * the {@link HttpClient} instance to be used for all requests. The provided httpClient should use a - * multi-threaded connection manager. If null, a default HttpClient will be used. - * @param lbSolrClient - * LBHttpSolrClient instance for requests. If null, a default LBHttpSolrClient will be used. - * @param lbHttpSolrClientBuilder - * LBHttpSolrClient builder to construct the LBHttpSolrClient. If null, a default builder will be used. - * @param updatesToLeaders - * If true, sends updates to shard leaders. - * @param directUpdatesToLeadersOnly - * If true, sends direct updates to shard leaders only. + * @param builder a {@link CloudSolrClient.Builder} with the options used to create the client. */ - private CloudSolrClient(Collection zkHosts, - String chroot, - List solrUrls, - HttpClient httpClient, - LBHttpSolrClient lbSolrClient, - LBHttpSolrClient.Builder lbHttpSolrClientBuilder, - boolean updatesToLeaders, - boolean directUpdatesToLeadersOnly, - ClusterStateProvider stateProvider - - ) { - if (stateProvider == null) { - if (zkHosts != null && solrUrls != null) { + protected CloudSolrClient(Builder builder) { + if (builder.stateProvider == null) { + if (builder.zkHosts != null && builder.solrUrls != null) { throw new IllegalArgumentException("Both zkHost(s) & solrUrl(s) have been specified. Only specify one."); } - if (zkHosts != null) { - this.stateProvider = new ZkClientClusterStateProvider(zkHosts, chroot); - } else if (solrUrls != null && !solrUrls.isEmpty()) { + if (builder.zkHosts != null) { + this.stateProvider = new ZkClientClusterStateProvider(builder.zkHosts, builder.zkChroot); + } else if (builder.solrUrls != null && !builder.solrUrls.isEmpty()) { try { - this.stateProvider = new HttpClusterStateProvider(solrUrls, httpClient); + this.stateProvider = new HttpClusterStateProvider(builder.solrUrls, builder.httpClient); } catch (Exception e) { throw new RuntimeException("Couldn't initialize a HttpClusterStateProvider (is/are the " - + "Solr server(s), " + solrUrls + ", down?)", e); + + "Solr server(s), " + builder.solrUrls + ", down?)", e); } } else { throw new IllegalArgumentException("Both zkHosts and solrUrl cannot be null."); } } else { - this.stateProvider = stateProvider; + this.stateProvider = builder.stateProvider; } - this.clientIsInternal = httpClient == null; - this.shutdownLBHttpSolrServer = lbSolrClient == null; - if(lbHttpSolrClientBuilder != null) lbSolrClient = lbHttpSolrClientBuilder.build(); - if(lbSolrClient != null) httpClient = lbSolrClient.getHttpClient(); - this.myClient = httpClient == null ? HttpClientUtil.createClient(null) : httpClient; - if (lbSolrClient == null) lbSolrClient = createLBHttpSolrClient(myClient); - this.lbClient = lbSolrClient; - this.updatesToLeaders = updatesToLeaders; - this.directUpdatesToLeadersOnly = directUpdatesToLeadersOnly; + this.clientIsInternal = builder.httpClient == null; + this.shutdownLBHttpSolrServer = builder.loadBalancedSolrClient == null; + if(builder.lbClientBuilder != null) builder.loadBalancedSolrClient = builder.lbClientBuilder.build(); + if(builder.loadBalancedSolrClient != null) builder.httpClient = builder.loadBalancedSolrClient.getHttpClient(); + this.myClient = (builder.httpClient == null) ? HttpClientUtil.createClient(null) : builder.httpClient; + if (builder.loadBalancedSolrClient == null) builder.loadBalancedSolrClient = createLBHttpSolrClient(myClient); + this.lbClient = builder.loadBalancedSolrClient; + this.updatesToLeaders = builder.shardLeadersOnly; + this.directUpdatesToLeadersOnly = builder.directUpdatesToLeadersOnly; } /**Sets the cache ttl for DocCollection Objects cached . This is only applicable for collections which are persisted outside of clusterstate.json @@ -1372,15 +1339,15 @@ public class CloudSolrClient extends SolrClient { * Constructs {@link CloudSolrClient} instances from provided configuration. */ public static class Builder { - private Collection zkHosts; - private List solrUrls; - private HttpClient httpClient; - private String zkChroot; - private LBHttpSolrClient loadBalancedSolrClient; - private LBHttpSolrClient.Builder lbClientBuilder; - private boolean shardLeadersOnly; - private boolean directUpdatesToLeadersOnly; - private ClusterStateProvider stateProvider; + protected Collection zkHosts; + protected List solrUrls; + protected HttpClient httpClient; + protected String zkChroot; + protected LBHttpSolrClient loadBalancedSolrClient; + protected LBHttpSolrClient.Builder lbClientBuilder; + protected boolean shardLeadersOnly; + protected boolean directUpdatesToLeadersOnly; + protected ClusterStateProvider stateProvider; public Builder() { @@ -1536,8 +1503,7 @@ public class CloudSolrClient extends SolrClient { throw new IllegalArgumentException("Both zkHosts and solrUrl cannot be null."); } } - return new CloudSolrClient(zkHosts, zkChroot, solrUrls, httpClient, loadBalancedSolrClient, lbClientBuilder, - shardLeadersOnly, directUpdatesToLeadersOnly, stateProvider); + return new CloudSolrClient(this); } } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java index f169a605217..fead54fdec1 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java @@ -97,33 +97,52 @@ public class ConcurrentUpdateSolrClient extends SolrClient { /** * Uses the supplied HttpClient to send documents to the Solr server. + * + * @deprecated use {@link ConcurrentUpdateSolrClient#ConcurrentUpdateSolrClient(Builder)} instead, as it is a more extension/subclassing-friendly alternative */ + @Deprecated protected ConcurrentUpdateSolrClient(String solrServerUrl, HttpClient client, int queueSize, int threadCount, ExecutorService es, boolean streamDeletes) { - this.internalHttpClient = (client == null); - this.client = new HttpSolrClient.Builder(solrServerUrl) + this((streamDeletes) ? + new Builder(solrServerUrl) .withHttpClient(client) + .withQueueSize(queueSize) + .withThreadCount(threadCount) + .withExecutorService(es) + .alwaysStreamDeletes() : + new Builder(solrServerUrl) + .withHttpClient(client) + .withQueueSize(queueSize) + .withThreadCount(threadCount) + .withExecutorService(es) + .neverStreamDeletes()); + } + + protected ConcurrentUpdateSolrClient(Builder builder) { + this.internalHttpClient = (builder.httpClient == null); + this.client = new HttpSolrClient.Builder(builder.baseSolrUrl) + .withHttpClient(builder.httpClient) .build(); this.client.setFollowRedirects(false); - queue = new LinkedBlockingQueue<>(queueSize); - this.threadCount = threadCount; - runners = new LinkedList<>(); - this.streamDeletes = streamDeletes; + this.queue = new LinkedBlockingQueue<>(builder.queueSize); + this.threadCount = builder.threadCount; + this.runners = new LinkedList<>(); + this.streamDeletes = builder.streamDeletes; - if (es != null) { - scheduler = es; - shutdownExecutor = false; + if (builder.executorService != null) { + this.scheduler = builder.executorService; + this.shutdownExecutor = false; } else { - scheduler = ExecutorUtil.newMDCAwareCachedThreadPool(new SolrjNamedThreadFactory("concurrentUpdateScheduler")); - shutdownExecutor = true; + this.scheduler = ExecutorUtil.newMDCAwareCachedThreadPool(new SolrjNamedThreadFactory("concurrentUpdateScheduler")); + this.shutdownExecutor = true; } if (log.isDebugEnabled()) { - pollInterrupts = new AtomicInteger(); - pollExits = new AtomicInteger(); - blockLoops = new AtomicInteger(); - emptyQueueLoops = new AtomicInteger(); + this.pollInterrupts = new AtomicInteger(); + this.pollExits = new AtomicInteger(); + this.blockLoops = new AtomicInteger(); + this.emptyQueueLoops = new AtomicInteger(); } } @@ -743,12 +762,12 @@ public class ConcurrentUpdateSolrClient extends SolrClient { * Constructs {@link ConcurrentUpdateSolrClient} instances from provided configuration. */ public static class Builder { - private String baseSolrUrl; - private HttpClient httpClient; - private int queueSize; - private int threadCount; - private ExecutorService executorService; - private boolean streamDeletes; + protected String baseSolrUrl; + protected HttpClient httpClient; + protected int queueSize; + protected int threadCount; + protected ExecutorService executorService; + protected boolean streamDeletes; /** * Create a Builder object, based on the provided Solr URL. @@ -838,7 +857,7 @@ public class ConcurrentUpdateSolrClient extends SolrClient { throw new IllegalArgumentException("Cannot create HttpSolrClient without a valid baseSolrUrl!"); } - return new ConcurrentUpdateSolrClient(baseSolrUrl, httpClient, queueSize, threadCount, executorService, streamDeletes); + return new ConcurrentUpdateSolrClient(this); } } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java index fc83391260a..e878110a3a3 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java @@ -38,7 +38,11 @@ public class DelegationTokenHttpSolrClient extends HttpSolrClient { * Package protected constructor for use by * {@linkplain org.apache.solr.client.solrj.impl.HttpSolrClient.Builder}. * @lucene.internal + * + * @deprecated use {@link DelegationTokenHttpSolrClient#DelegationTokenHttpSolrClient(HttpSolrClient.Builder)} instead, as it is a more + * extension/subclassing-friendly alternative */ + @Deprecated DelegationTokenHttpSolrClient(String baseURL, HttpClient client, ResponseParser parser, @@ -52,6 +56,11 @@ public class DelegationTokenHttpSolrClient extends HttpSolrClient { invariantParams = new ModifiableSolrParams(); invariantParams.set(DELEGATION_TOKEN_PARAM, delegationToken); } + + protected DelegationTokenHttpSolrClient(Builder builder) { + super(builder); + setQueryParams(new TreeSet<>(Arrays.asList(DELEGATION_TOKEN_PARAM))); + } /** * This constructor is defined at "protected" scope. Ideally applications should @@ -63,7 +72,11 @@ public class DelegationTokenHttpSolrClient extends HttpSolrClient { * @param parser Response parser instance to use to decode response from Solr server * @param allowCompression Should compression be allowed ? * @param invariantParams The parameters which should be passed with every request. + * + * @deprecated use {@link DelegationTokenHttpSolrClient#DelegationTokenHttpSolrClient(HttpSolrClient.Builder)} instead, as it is a more + * extension/subclassing-friendly alternative */ + @Deprecated protected DelegationTokenHttpSolrClient(String baseURL, HttpClient client, ResponseParser parser, diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index e73e08b34e7..f944d7cd8dc 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -143,8 +143,40 @@ public class HttpSolrClient extends SolrClient { private volatile Integer connectionTimeout; private volatile Integer soTimeout; + /** + * @deprecated use {@link HttpSolrClient#HttpSolrClient(Builder)} instead, as it is a more extension/subclassing-friendly alternative + */ + @Deprecated protected HttpSolrClient(String baseURL, HttpClient client, ResponseParser parser, boolean allowCompression) { - this.baseUrl = baseURL; + this(new Builder(baseURL) + .withHttpClient(client) + .withResponseParser(parser) + .allowCompression(allowCompression)); + } + + /** + * The constructor. + * + * @param baseURL The base url to communicate with the Solr server + * @param client Http client instance to use for communication + * @param parser Response parser instance to use to decode response from Solr server + * @param allowCompression Should compression be allowed ? + * @param invariantParams The parameters which should be included with every request. + * + * @deprecated use {@link HttpSolrClient#HttpSolrClient(Builder)} instead, as it is a more extension/subclassing-friendly alternative + */ + @Deprecated + protected HttpSolrClient(String baseURL, HttpClient client, ResponseParser parser, boolean allowCompression, + ModifiableSolrParams invariantParams) { + this(new Builder(baseURL) + .withHttpClient(client) + .withResponseParser(parser) + .allowCompression(allowCompression) + .withInvariantParams(invariantParams)); + } + + protected HttpSolrClient(Builder builder) { + this.baseUrl = builder.baseSolrUrl; if (baseUrl.endsWith("/")) { baseUrl = baseUrl.substring(0, baseUrl.length() - 1); } @@ -154,34 +186,19 @@ public class HttpSolrClient extends SolrClient { + baseUrl); } - if (client != null) { - httpClient = client; - internalClient = false; + if (builder.httpClient != null) { + this.httpClient = builder.httpClient; + this.internalClient = false; } else { - internalClient = true; + this.internalClient = true; ModifiableSolrParams params = new ModifiableSolrParams(); params.set(HttpClientUtil.PROP_FOLLOW_REDIRECTS, followRedirects); - params.set(HttpClientUtil.PROP_ALLOW_COMPRESSION, allowCompression); + params.set(HttpClientUtil.PROP_ALLOW_COMPRESSION, builder.compression); httpClient = HttpClientUtil.createClient(params); } - this.parser = parser; - } - - /** - * The consturctor. - * - * @param baseURL The base url to communicate with the Solr server - * @param client Http client instance to use for communication - * @param parser Response parser instance to use to decode response from Solr server - * @param allowCompression Should compression be allowed ? - * @param invariantParams The parameters which should be included with every request. - */ - protected HttpSolrClient(String baseURL, HttpClient client, ResponseParser parser, boolean allowCompression, - ModifiableSolrParams invariantParams) { - this(baseURL, client, parser, allowCompression); - - this.invariantParams = invariantParams; + this.parser = builder.responseParser; + this.invariantParams = builder.invariantParams; } public Set getQueryParams() { @@ -803,11 +820,11 @@ public class HttpSolrClient extends SolrClient { * Constructs {@link HttpSolrClient} instances from provided configuration. */ public static class Builder { - private String baseSolrUrl; - private HttpClient httpClient; - private ResponseParser responseParser; - private boolean compression; - private ModifiableSolrParams invariantParams = new ModifiableSolrParams(); + protected String baseSolrUrl; + protected HttpClient httpClient; + protected ResponseParser responseParser; + protected boolean compression; + protected ModifiableSolrParams invariantParams = new ModifiableSolrParams(); public Builder() { this.responseParser = new BinaryResponseParser(); @@ -924,9 +941,9 @@ public class HttpSolrClient extends SolrClient { } if (this.invariantParams.get(DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM) == null) { - return new HttpSolrClient(baseSolrUrl, httpClient, responseParser, compression, invariantParams); + return new HttpSolrClient(this); } else { - return new DelegationTokenHttpSolrClient(baseSolrUrl, httpClient, responseParser, compression, invariantParams); + return new DelegationTokenHttpSolrClient(this); } } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java index dbb99cea7c9..b96c9353341 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java @@ -239,33 +239,43 @@ public class LBHttpSolrClient extends SolrClient { /** * The provided httpClient should use a multi-threaded connection manager + * + * @deprecated use {@link LBHttpSolrClient#LBHttpSolrClient(Builder)} instead, as it is a more extension/subclassing-friendly alternative */ + @Deprecated protected LBHttpSolrClient(HttpSolrClient.Builder httpSolrClientBuilder, HttpClient httpClient, String... solrServerUrl) { - clientIsInternal = httpClient == null; - this.httpSolrClientBuilder = httpSolrClientBuilder; - httpClient = constructClient(null); - this.httpClient = httpClient; - if (solrServerUrl != null) { - for (String s : solrServerUrl) { - ServerWrapper wrapper = new ServerWrapper(makeSolrClient(s)); - aliveServers.put(wrapper.getKey(), wrapper); - } - } - updateAliveList(); + this(new Builder() + .withHttpSolrClientBuilder(httpSolrClientBuilder) + .withHttpClient(httpClient) + .withBaseSolrUrls(solrServerUrl)); } /** * The provided httpClient should use a multi-threaded connection manager + * + * @deprecated use {@link LBHttpSolrClient#LBHttpSolrClient(Builder)} instead, as it is a more extension/subclassing-friendly alternative */ + @Deprecated protected LBHttpSolrClient(HttpClient httpClient, ResponseParser parser, String... solrServerUrl) { - clientIsInternal = (httpClient == null); - this.httpClient = httpClient == null ? constructClient(solrServerUrl) : httpClient; - this.parser = parser; + this(new Builder() + .withBaseSolrUrls(solrServerUrl) + .withResponseParser(parser) + .withHttpClient(httpClient)); + } + + protected LBHttpSolrClient(Builder builder) { + this.clientIsInternal = builder.httpClient == null; + this.httpSolrClientBuilder = builder.httpSolrClientBuilder; + this.httpClient = builder.httpClient == null ? constructClient(builder.baseSolrUrls.toArray(new String[builder.baseSolrUrls.size()])) : builder.httpClient; - for (String s : solrServerUrl) { - ServerWrapper wrapper = new ServerWrapper(makeSolrClient(s)); - aliveServers.put(wrapper.getKey(), wrapper); + this.parser = builder.responseParser; + + if (! builder.baseSolrUrls.isEmpty()) { + for (String s : builder.baseSolrUrls) { + ServerWrapper wrapper = new ServerWrapper(makeSolrClient(s)); + aliveServers.put(wrapper.getKey(), wrapper); + } } updateAliveList(); } @@ -852,10 +862,10 @@ public class LBHttpSolrClient extends SolrClient { * Constructs {@link LBHttpSolrClient} instances from provided configuration. */ public static class Builder { - private final List baseSolrUrls; - private HttpClient httpClient; - private ResponseParser responseParser; - private HttpSolrClient.Builder httpSolrClientBuilder; + protected final List baseSolrUrls; + protected HttpClient httpClient; + protected ResponseParser responseParser; + protected HttpSolrClient.Builder httpSolrClientBuilder; public Builder() { this.baseSolrUrls = new ArrayList<>(); @@ -953,13 +963,7 @@ public class LBHttpSolrClient extends SolrClient { * Create a {@link HttpSolrClient} based on provided configuration. */ public LBHttpSolrClient build() { - final String[] baseUrlArray = new String[baseSolrUrls.size()]; - String[] solrServerUrls = baseSolrUrls.toArray(baseUrlArray); - if (httpSolrClientBuilder != null) { - return new LBHttpSolrClient(httpSolrClientBuilder, httpClient, solrServerUrls); - } else { - return new LBHttpSolrClient(httpClient, responseParser, solrServerUrls); - } + return new LBHttpSolrClient(this); } } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingTest.java index c2314f8b33f..35ab8983bf9 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingTest.java @@ -39,25 +39,10 @@ import java.util.List; @Slow public class SolrExampleStreamingTest extends SolrExampleTests { - protected Throwable handledException = null; - @BeforeClass public static void beforeTest() throws Exception { createJetty(legacyExampleCollection1SolrHome()); } - - public class ErrorTrackingConcurrentUpdateSolrClient extends ConcurrentUpdateSolrClient { - public Throwable lastError = null; - - public ErrorTrackingConcurrentUpdateSolrClient(String solrServerUrl, int queueSize, int threadCount) { - super(solrServerUrl, null, queueSize, threadCount, null, false); - } - - @Override - public void handleError(Throwable ex) { - handledException = lastError = ex; - } - } @Override public SolrClient createNewSolrClient() @@ -66,7 +51,10 @@ public class SolrExampleStreamingTest extends SolrExampleTests { // setup the server... String url = jetty.getBaseUrl().toString() + "/collection1"; // smaller queue size hits locks more often - ConcurrentUpdateSolrClient concurrentClient = new ErrorTrackingConcurrentUpdateSolrClient( url, 2, 5 ); + ConcurrentUpdateSolrClient concurrentClient = new ErrorTrackingConcurrentUpdateSolrClient.Builder(url) + .withQueueSize(2) + .withThreadCount(5) + .build(); concurrentClient.setParser(new XMLResponseParser()); concurrentClient.setRequestWriter(new RequestWriter()); return concurrentClient; @@ -81,7 +69,10 @@ public class SolrExampleStreamingTest extends SolrExampleTests { // SOLR-3903 final List failures = new ArrayList<>(); final String serverUrl = jetty.getBaseUrl().toString() + "/collection1"; - try (ConcurrentUpdateSolrClient concurrentClient = new FailureRecordingConcurrentUpdateSolrClient(serverUrl, 2, 2)) { + try (ConcurrentUpdateSolrClient concurrentClient = new FailureRecordingConcurrentUpdateSolrClient.Builder(serverUrl) + .withQueueSize(2) + .withThreadCount(2) + .build()) { int docId = 42; for (UpdateRequest.ACTION action : EnumSet.allOf(UpdateRequest.ACTION.class)) { for (boolean waitSearch : Arrays.asList(true, false)) { @@ -108,14 +99,49 @@ public class SolrExampleStreamingTest extends SolrExampleTests { static class FailureRecordingConcurrentUpdateSolrClient extends ConcurrentUpdateSolrClient { private final List failures = new ArrayList<>(); - public FailureRecordingConcurrentUpdateSolrClient(String serverUrl, int queueSize, int numThreads) { - super(serverUrl, null, queueSize, numThreads, null, false); + public FailureRecordingConcurrentUpdateSolrClient(Builder builder) { + super(builder); } @Override public void handleError(Throwable ex) { failures.add(ex); } + + static class Builder extends ConcurrentUpdateSolrClient.Builder { + public Builder(String baseSolrUrl) { + super(baseSolrUrl); + } + + @Override + public FailureRecordingConcurrentUpdateSolrClient build() { + return new FailureRecordingConcurrentUpdateSolrClient(this); + } + } } + + public static class ErrorTrackingConcurrentUpdateSolrClient extends ConcurrentUpdateSolrClient { + public Throwable lastError = null; + + public ErrorTrackingConcurrentUpdateSolrClient(Builder builder) { + super(builder); + } + + @Override + public void handleError(Throwable ex) { + lastError = ex; + } + + static class Builder extends ConcurrentUpdateSolrClient.Builder { + public Builder(String baseSolrUrl) { + super(baseSolrUrl); + } + + @Override + public ErrorTrackingConcurrentUpdateSolrClient build() { + return new ErrorTrackingConcurrentUpdateSolrClient(this); + } + } + } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTest.java index 4b061d54e6e..44afccd5c6d 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClientTest.java @@ -148,8 +148,10 @@ public class ConcurrentUpdateSolrClientTest extends SolrJettyTestBase { final StringBuilder errors = new StringBuilder(); @SuppressWarnings("serial") - ConcurrentUpdateSolrClient concurrentClient = new OutcomeCountingConcurrentUpdateSolrClient(serverUrl, cussQueueSize, - cussThreadCount, successCounter, errorCounter, errors); + ConcurrentUpdateSolrClient concurrentClient = new OutcomeCountingConcurrentUpdateSolrClient.Builder(serverUrl, successCounter, errorCounter, errors) + .withQueueSize(cussQueueSize) + .withThreadCount(cussThreadCount) + .build(); concurrentClient.setPollQueueTime(0); @@ -309,13 +311,12 @@ public class ConcurrentUpdateSolrClientTest extends SolrJettyTestBase { private final AtomicInteger successCounter; private final AtomicInteger failureCounter; private final StringBuilder errors; - public OutcomeCountingConcurrentUpdateSolrClient(String serverUrl, int queueSize, int threadCount, - AtomicInteger successCounter, AtomicInteger failureCounter, StringBuilder errors) { - super(serverUrl, null, queueSize, threadCount, null, false); - - this.successCounter = successCounter; - this.failureCounter = failureCounter; - this.errors = errors; + + public OutcomeCountingConcurrentUpdateSolrClient(Builder builder) { + super(builder); + this.successCounter = builder.successCounter; + this.failureCounter = builder.failureCounter; + this.errors = builder.errors; } @Override @@ -327,5 +328,22 @@ public class ConcurrentUpdateSolrClientTest extends SolrJettyTestBase { public void onSuccess(HttpResponse resp) { successCounter.incrementAndGet(); } + + static class Builder extends ConcurrentUpdateSolrClient.Builder { + protected final AtomicInteger successCounter; + protected final AtomicInteger failureCounter; + protected final StringBuilder errors; + + public Builder(String baseSolrUrl, AtomicInteger successCounter, AtomicInteger failureCounter, StringBuilder errors) { + super(baseSolrUrl); + this.successCounter = successCounter; + this.failureCounter = failureCounter; + this.errors = errors; + } + + public OutcomeCountingConcurrentUpdateSolrClient build() { + return new OutcomeCountingConcurrentUpdateSolrClient(this); + } + } } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientTest.java index 94af2ca26ce..34804c408d6 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientTest.java @@ -40,7 +40,7 @@ public class LBHttpSolrClientTest { public void testLBHttpSolrClientHttpClientResponseParserStringArray() throws IOException { CloseableHttpClient httpClient = HttpClientUtil.createClient(new ModifiableSolrParams()); try ( - LBHttpSolrClient testClient = new LBHttpSolrClient(httpClient, (ResponseParser) null); + LBHttpSolrClient testClient = new LBHttpSolrClient.Builder().withHttpClient(httpClient).withResponseParser(null).build(); HttpSolrClient httpSolrClient = testClient.makeSolrClient("http://127.0.0.1:8080")) { assertNull("Generated server should have null parser.", httpSolrClient.getParser()); } finally {