dsmiley commented on code in PR #1835:
URL: https://github.com/apache/solr/pull/1835#discussion_r1291808504
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -40,85 +43,131 @@
*
* <p>TODO: Cut this over to using Solr's new Http2 clients
Review Comment:
Could remove this javadoc line; we know we want to remove legacy clients and
we are starting to use newer clients here now (thank you).
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -40,85 +43,131 @@
*
* <p>TODO: Cut this over to using Solr's new Http2 clients
*/
-public class SolrClientCache implements Serializable {
+public class SolrClientCache implements Serializable, Closeable {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private final Map<String, SolrClient> solrClients = new HashMap<>();
- private final HttpClient httpClient;
// Set the floor for timeouts to 60 seconds.
// Timeouts cans be increased by setting the system properties defined below.
- private static final int conTimeout =
- Math.max(
-
Integer.parseInt(System.getProperty(HttpClientUtil.PROP_CONNECTION_TIMEOUT,
"60000")),
- 60000);
- private static final int socketTimeout =
+ private static final int MIN_TIMEOUT = 60000;
+ private static final int minConnTimeout =
Math.max(
- Integer.parseInt(System.getProperty(HttpClientUtil.PROP_SO_TIMEOUT,
"60000")), 60000);
+ Integer.getInteger(HttpClientUtil.PROP_CONNECTION_TIMEOUT,
MIN_TIMEOUT), MIN_TIMEOUT);
+ private static final int minSocketTimeout =
+ Math.max(Integer.getInteger(HttpClientUtil.PROP_SO_TIMEOUT,
MIN_TIMEOUT), MIN_TIMEOUT);
+
+ private final Map<String, SolrClient> solrClients = new HashMap<>();
+ private final HttpClient httpClient;
Review Comment:
maybe we should name this field apacheHttpClient to clarify *which* type of
HttpClient it is?
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/Facet2DStream.java:
##########
@@ -325,13 +322,7 @@ public void open() throws IOException {
if (cache != null) {
cloudSolrClient = cache.getCloudSolrClient(zkHost);
} else {
- final List<String> hosts = new ArrayList<>();
- hosts.add(zkHost);
- cloudSolrClient =
- new CloudLegacySolrClient.Builder(hosts, Optional.empty())
- .withSocketTimeout(30000, TimeUnit.MILLISECONDS)
- .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
- .build();
+ cloudSolrClient = SolrClientCache.newCloudHttp2SolrClient(zkHost, null);
Review Comment:
This line is curious... it seems you have made a public static method on
SolrClientCache for creating a CloudHttp2SolrClient, thus making this class a
builder for anyone anywhere who wants such a client? Shouldn't we let the
builder of CloudSolrClient do such a job?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java:
##########
@@ -829,6 +824,7 @@ private NamedList<Object> processErrorsAndResponse(
// no processor specified, return raw stream
NamedList<Object> rsp = new NamedList<>();
rsp.add("stream", is);
+ rsp.add("responseStatus", httpStatus);
Review Comment:
Maybe we should name this "httpStatus" and should use an integer so as to
keep generic of the type of HttpClient we are using?
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -40,85 +43,131 @@
*
* <p>TODO: Cut this over to using Solr's new Http2 clients
*/
-public class SolrClientCache implements Serializable {
+public class SolrClientCache implements Serializable, Closeable {
Review Comment:
Maybe a subclass approach would work better, such that LegacySolrClientCache
(proposed name) would override production of a new SolrClient? After all, we
have multiple types of SolrClient implementations without having a single one
trying to work with both HttpClient types.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java:
##########
@@ -40,85 +43,131 @@
*
* <p>TODO: Cut this over to using Solr's new Http2 clients
*/
-public class SolrClientCache implements Serializable {
+public class SolrClientCache implements Serializable, Closeable {
Review Comment:
While we are touching this, do we really need to implement Serializable?!
--
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
