dsmiley commented on code in PR #2899:
URL: https://github.com/apache/solr/pull/2899#discussion_r1876531872
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -94,35 +97,74 @@
*
* @since solr 8.0
*/
-public class LBHttp2SolrClient<C extends HttpSolrClientBase> extends
LBSolrClient {
+public class LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>>
extends LBSolrClient {
- protected final C solrClient;
+ private final Map<String, HttpSolrClientBase> urlToClient;
+ private final Set<String> urlParamNames;
+
+ private final HttpSolrClientBuilderBase<?, ?> solrClientBuilder;
@SuppressWarnings("unchecked")
private LBHttp2SolrClient(Builder<?> builder) {
super(Arrays.asList(builder.solrEndpoints));
- this.solrClient = (C) builder.solrClient;
+ this.solrClientBuilder = builder.solrClientBuilder;
+
+ this.urlToClient = new ConcurrentHashMap<>();
+ for (LBSolrClient.Endpoint endpoint : builder.solrEndpoints) {
+ buildClient(endpoint);
+ }
+
this.aliveCheckIntervalMillis = builder.aliveCheckIntervalMillis;
this.defaultCollection = builder.defaultCollection;
+
+ if (builder.solrClientBuilder.urlParamNames == null) {
+ this.urlParamNames = Collections.emptySet();
+ } else {
+ this.urlParamNames =
Collections.unmodifiableSet(builder.solrClientBuilder.urlParamNames);
+ }
+ }
+
+ private synchronized HttpSolrClientBase buildClient(Endpoint endpoint) {
+ var client = urlToClient.get(endpoint.toString());
+ if (client == null) {
+ String tmpBaseSolrUrl = solrClientBuilder.baseSolrUrl;
+ solrClientBuilder.baseSolrUrl = endpoint.getBaseUrl();
+ client = solrClientBuilder.build();
+ urlToClient.put(endpoint.getBaseUrl(), client);
+ solrClientBuilder.baseSolrUrl = tmpBaseSolrUrl;
+ }
+ return client;
Review Comment:
Prefer calling ConcurrentHashMap.computeIfAbsent or similar to get-then-put
because of it's nice atomicity properties, and avoids synchronization needs
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -94,35 +97,74 @@
*
* @since solr 8.0
*/
-public class LBHttp2SolrClient<C extends HttpSolrClientBase> extends
LBSolrClient {
+public class LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>>
extends LBSolrClient {
- protected final C solrClient;
+ private final Map<String, HttpSolrClientBase> urlToClient;
+ private final Set<String> urlParamNames;
+
+ private final HttpSolrClientBuilderBase<?, ?> solrClientBuilder;
@SuppressWarnings("unchecked")
private LBHttp2SolrClient(Builder<?> builder) {
super(Arrays.asList(builder.solrEndpoints));
- this.solrClient = (C) builder.solrClient;
+ this.solrClientBuilder = builder.solrClientBuilder;
+
+ this.urlToClient = new ConcurrentHashMap<>();
+ for (LBSolrClient.Endpoint endpoint : builder.solrEndpoints) {
+ buildClient(endpoint);
+ }
+
this.aliveCheckIntervalMillis = builder.aliveCheckIntervalMillis;
this.defaultCollection = builder.defaultCollection;
+
+ if (builder.solrClientBuilder.urlParamNames == null) {
+ this.urlParamNames = Collections.emptySet();
+ } else {
+ this.urlParamNames =
Collections.unmodifiableSet(builder.solrClientBuilder.urlParamNames);
Review Comment:
probably fine but I'd prefer Set.copyOf here
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -94,35 +97,74 @@
*
* @since solr 8.0
*/
-public class LBHttp2SolrClient<C extends HttpSolrClientBase> extends
LBSolrClient {
+public class LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>>
extends LBSolrClient {
- protected final C solrClient;
+ private final Map<String, HttpSolrClientBase> urlToClient;
+ private final Set<String> urlParamNames;
+
+ private final HttpSolrClientBuilderBase<?, ?> solrClientBuilder;
@SuppressWarnings("unchecked")
private LBHttp2SolrClient(Builder<?> builder) {
super(Arrays.asList(builder.solrEndpoints));
- this.solrClient = (C) builder.solrClient;
+ this.solrClientBuilder = builder.solrClientBuilder;
+
+ this.urlToClient = new ConcurrentHashMap<>();
+ for (LBSolrClient.Endpoint endpoint : builder.solrEndpoints) {
+ buildClient(endpoint);
+ }
+
this.aliveCheckIntervalMillis = builder.aliveCheckIntervalMillis;
this.defaultCollection = builder.defaultCollection;
+
+ if (builder.solrClientBuilder.urlParamNames == null) {
+ this.urlParamNames = Collections.emptySet();
+ } else {
+ this.urlParamNames =
Collections.unmodifiableSet(builder.solrClientBuilder.urlParamNames);
+ }
+ }
+
+ private synchronized HttpSolrClientBase buildClient(Endpoint endpoint) {
Review Comment:
synchronized is guarding what here?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttp2SolrClient.java:
##########
@@ -94,35 +97,74 @@
*
* @since solr 8.0
*/
-public class LBHttp2SolrClient<C extends HttpSolrClientBase> extends
LBSolrClient {
+public class LBHttp2SolrClient<B extends HttpSolrClientBuilderBase<?, ?>>
extends LBSolrClient {
- protected final C solrClient;
+ private final Map<String, HttpSolrClientBase> urlToClient;
+ private final Set<String> urlParamNames;
+
+ private final HttpSolrClientBuilderBase<?, ?> solrClientBuilder;
@SuppressWarnings("unchecked")
private LBHttp2SolrClient(Builder<?> builder) {
super(Arrays.asList(builder.solrEndpoints));
- this.solrClient = (C) builder.solrClient;
+ this.solrClientBuilder = builder.solrClientBuilder;
+
+ this.urlToClient = new ConcurrentHashMap<>();
+ for (LBSolrClient.Endpoint endpoint : builder.solrEndpoints) {
+ buildClient(endpoint);
+ }
+
this.aliveCheckIntervalMillis = builder.aliveCheckIntervalMillis;
this.defaultCollection = builder.defaultCollection;
+
+ if (builder.solrClientBuilder.urlParamNames == null) {
+ this.urlParamNames = Collections.emptySet();
+ } else {
+ this.urlParamNames =
Collections.unmodifiableSet(builder.solrClientBuilder.urlParamNames);
+ }
+ }
+
+ private synchronized HttpSolrClientBase buildClient(Endpoint endpoint) {
+ var client = urlToClient.get(endpoint.toString());
+ if (client == null) {
+ String tmpBaseSolrUrl = solrClientBuilder.baseSolrUrl;
+ solrClientBuilder.baseSolrUrl = endpoint.getBaseUrl();
+ client = solrClientBuilder.build();
+ urlToClient.put(endpoint.getBaseUrl(), client);
+ solrClientBuilder.baseSolrUrl = tmpBaseSolrUrl;
+ }
+ return client;
}
@Override
- protected SolrClient getClient(Endpoint endpoint) {
- return solrClient;
+ protected HttpSolrClientBase getClient(Endpoint endpoint) {
+ var client = urlToClient.get(endpoint.getBaseUrl());
+ if (client == null) {
+ return buildClient(endpoint);
+ }
+ return client;
}
@Override
public ResponseParser getParser() {
- return solrClient.getParser();
+ return urlToClient.isEmpty() ? null :
urlToClient.values().iterator().next().getParser();
}
@Override
public RequestWriter getRequestWriter() {
- return solrClient.getRequestWriter();
+ return urlToClient.isEmpty() ? null :
urlToClient.values().iterator().next().getRequestWriter();
}
public Set<String> getUrlParamNames() {
- return solrClient.getUrlParamNames();
+ return urlParamNames;
+ }
+
+ @Override
+ public void close() {
+ super.close();
+ for (HttpSolrClientBase client : urlToClient.values()) {
+ IOUtils.closeQuietly(client);
+ }
Review Comment:
could be a one-liner with
`urlToClient.values().forEach(IOUtils::closeQuiety)`
--
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
