dsmiley commented on code in PR #2689:
URL: https://github.com/apache/solr/pull/2689#discussion_r1750378062
##########
solr/core/src/java/org/apache/solr/core/HttpSolrClientProvider.java:
##########
@@ -40,107 +34,55 @@
*
* @lucene.internal
*/
-class HttpSolrClientProvider implements SolrMetricProducer {
+class HttpSolrClientProvider {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static final String METRIC_SCOPE_NAME =
"defaultHttpSolrClientProvider";
+ static final String METRIC_SCOPE_NAME = "defaultHttpSolrClientProvider";
private final Http2SolrClient httpSolrClient;
private final InstrumentedHttpListenerFactory trackHttpSolrMetrics;
- private SolrMetricsContext solrMetricsContext;
-
- private int socketTimeout;
-
- private int connectionTimeout;
-
- public HttpSolrClientProvider(UpdateShardHandlerConfig cfg) {
- Http2SolrClient.Builder httpClientBuilder = new Http2SolrClient.Builder();
+ HttpSolrClientProvider(UpdateShardHandlerConfig cfg) {
trackHttpSolrMetrics = new
InstrumentedHttpListenerFactory(getNameStrategy(cfg));
- configureTimeouts(cfg);
-
- httpClientBuilder
- .withConnectionTimeout(connectionTimeout, TimeUnit.MILLISECONDS)
- .withIdleTimeout(socketTimeout, TimeUnit.MILLISECONDS)
- .withListenerFactory(List.of(trackHttpSolrMetrics));
-
- httpSolrClient = httpClientBuilder.build();
- }
-
- private void configureTimeouts(UpdateShardHandlerConfig cfg) {
- socketTimeout = HttpClientUtil.DEFAULT_SO_TIMEOUT;
- connectionTimeout = HttpClientUtil.DEFAULT_CONNECT_TIMEOUT;
+ Http2SolrClient.Builder httpClientBuilder =
+ new
Http2SolrClient.Builder().withListenerFactory(List.of(trackHttpSolrMetrics));
if (cfg != null) {
- socketTimeout = Math.max(cfg.getDistributedSocketTimeout(),
socketTimeout);
- connectionTimeout = Math.max(cfg.getDistributedConnectionTimeout(),
connectionTimeout);
+ httpClientBuilder
+ .withConnectionTimeout(cfg.getDistributedConnectionTimeout(),
TimeUnit.MILLISECONDS)
+ .withIdleTimeout(cfg.getDistributedSocketTimeout(),
TimeUnit.MILLISECONDS);
}
- }
-
- private HttpClientMetricNameStrategy
getMetricNameStrategy(UpdateShardHandlerConfig cfg) {
- HttpClientMetricNameStrategy metricNameStrategy =
-
KNOWN_METRIC_NAME_STRATEGIES.get(UpdateShardHandlerConfig.DEFAULT_METRICNAMESTRATEGY);
- if (cfg != null) {
- metricNameStrategy =
KNOWN_METRIC_NAME_STRATEGIES.get(cfg.getMetricNameStrategy());
- if (metricNameStrategy == null) {
- throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR,
- "Unknown metricNameStrategy: "
- + cfg.getMetricNameStrategy()
- + " found. Must be one of: "
- + KNOWN_METRIC_NAME_STRATEGIES.keySet());
- }
- }
- return metricNameStrategy;
+ httpSolrClient = httpClientBuilder.build();
}
private InstrumentedHttpListenerFactory.NameStrategy getNameStrategy(
UpdateShardHandlerConfig cfg) {
- InstrumentedHttpListenerFactory.NameStrategy nameStrategy =
- InstrumentedHttpListenerFactory.KNOWN_METRIC_NAME_STRATEGIES.get(
- UpdateShardHandlerConfig.DEFAULT_METRICNAMESTRATEGY);
-
- if (cfg != null) {
- nameStrategy =
- InstrumentedHttpListenerFactory.KNOWN_METRIC_NAME_STRATEGIES.get(
- cfg.getMetricNameStrategy());
- if (nameStrategy == null) {
- throw new SolrException(
- SolrException.ErrorCode.SERVER_ERROR,
- "Unknown metricNameStrategy: "
- + cfg.getMetricNameStrategy()
- + " found. Must be one of: "
- + KNOWN_METRIC_NAME_STRATEGIES.keySet());
- }
- }
- return nameStrategy;
+ String metricNameStrategy =
+ cfg != null && cfg.getMetricNameStrategy() != null
+ ? cfg.getMetricNameStrategy()
+ : UpdateShardHandlerConfig.DEFAULT_METRICNAMESTRATEGY;
+ return InstrumentedHttpListenerFactory.getNameStrategy(metricNameStrategy);
}
- public Http2SolrClient getSolrClient() {
+ Http2SolrClient getSolrClient() {
return httpSolrClient;
}
- public void setSecurityBuilder(HttpClientBuilderPlugin builder) {
+ void setSecurityBuilder(HttpClientBuilderPlugin builder) {
builder.setup(httpSolrClient);
}
- @Override
- public void initializeMetrics(SolrMetricsContext parentContext, String
scope) {
- solrMetricsContext = parentContext.getChildContext(this);
- String expandedScope = SolrMetricManager.mkName(scope,
SolrInfoBean.Category.HTTP.name());
+ void initializeMetrics(SolrMetricsContext parentContext) {
+ var solrMetricsContext = parentContext.getChildContext(this);
+ String expandedScope =
+ SolrMetricManager.mkName(METRIC_SCOPE_NAME,
SolrInfoBean.Category.HTTP.name());
trackHttpSolrMetrics.initializeMetrics(solrMetricsContext, expandedScope);
}
- @Override
- public SolrMetricsContext getSolrMetricsContext() {
- return solrMetricsContext;
- }
-
- @Override
- public void close() {
+ void close() {
try {
- SolrMetricProducer.super.close();
+ trackHttpSolrMetrics.close();
} catch (Exception e) {
log.error("Error closing SolrMetricProducer", e);
}
Review Comment:
we have IOUtils.closeQuietly
##########
solr/core/src/test/org/apache/solr/core/TestCoreContainer.java:
##########
@@ -1131,6 +1132,48 @@ public void testCoreInitFailuresOnReload() throws
Exception {
cc.shutdown();
}
+ public void testCustomSocketTimeoutForDefaultHttpClient() throws Exception {
Review Comment:
hmm; why this test here?
--
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
