----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28393/#review62902 -----------------------------------------------------------
It seems strange to me that the configuration here is all done via SolrXml; getting rid of the stale performance check and closing idle connections doesn't seem much different than configuring say, SO_TIMEOUT, which is all in HttpClientUtil. Also, if this configuration is so great, wouldn't I want it on my own HttpSolrServers, or at least the ability to configure it? Of course the parameters could still be set in SolrConfigXml for the HttpShardhandler and UpdateShardHandler, but the meat of it seems like it could live in HttpClientUtil. One complication with that approach is how to close the pool. Perhaps, given we are about to do a new major release, it's a good time to update our interfaces to return CloseableHttpClients rather than HttpClients. Just a thought. trunk/solr/core/src/java/org/apache/solr/core/ConfigSolr.java <https://reviews.apache.org/r/28393/#comment105078> is there anywhere that says what the unit of time this actually is? What about just renaming TIME -> SECS or something? trunk/solr/core/src/java/org/apache/solr/core/CoreContainer.java <https://reviews.apache.org/r/28393/#comment105056> remove. trunk/solr/core/src/java/org/apache/solr/core/CoreContainer.java <https://reviews.apache.org/r/28393/#comment105065> why isn't this the same as above, i.e. 2 passed to the hashmap constructor. Actually, could this whole thing be simplified, i.e. if (info == null || info.attributes == null || info.attributes.size==0) { attributes = new ... } else { attributes = info.attributes; } trunk/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java <https://reviews.apache.org/r/28393/#comment105070> should these be configurable? At least TCP_NODELAY seems like it should, it seems orthogonal to the rest of the patch. trunk/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java <https://reviews.apache.org/r/28393/#comment105071> So we don't do anything if we don't use the default client? This seems like a better fit for the HttpClientUtil. trunk/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java <https://reviews.apache.org/r/28393/#comment105077> empty finally? trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java <https://reviews.apache.org/r/28393/#comment105054> These seem spurious. - Gregory Chanan On Nov. 24, 2014, 3:42 p.m., Mark Miller wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28393/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2014, 3:42 p.m.) > > > Review request for lucene. > > > Repository: lucene > > > Description > ------- > > https://issues.apache.org/jira/browse/SOLR-4509 > > > Diffs > ----- > > trunk/solr/core/src/java/org/apache/solr/core/ConfigSolr.java 1641405 > trunk/solr/core/src/java/org/apache/solr/core/ConfigSolrXml.java 1641405 > trunk/solr/core/src/java/org/apache/solr/core/ConfigSolrXmlOld.java 1641405 > trunk/solr/core/src/java/org/apache/solr/core/CoreContainer.java 1641405 > trunk/solr/core/src/java/org/apache/solr/core/PluginInfo.java 1641405 > > trunk/solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java > 1641405 > trunk/solr/core/src/java/org/apache/solr/update/UpdateShardHandler.java > 1641405 > > trunk/solr/core/src/java/org/apache/solr/util/IdleConnectionMonitorRunnable.java > PRE-CREATION > trunk/solr/core/src/test/org/apache/solr/cloud/OverseerTest.java 1641405 > > trunk/solr/core/src/test/org/apache/solr/cloud/TestLeaderElectionZkExpiry.java > 1641405 > trunk/solr/core/src/test/org/apache/solr/cloud/TestZkChroot.java 1641405 > trunk/solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java > 1641405 > trunk/solr/core/src/test/org/apache/solr/core/TestCoreDiscovery.java > 1641405 > > trunk/solr/core/src/test/org/apache/solr/core/TestImplicitCoreProperties.java > 1641405 > trunk/solr/server/etc/jetty.xml 1641405 > > trunk/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java > 1641405 > > Diff: https://reviews.apache.org/r/28393/diff/ > > > Testing > ------- > > > Thanks, > > Mark Miller > >
