Copilot commented on code in PR #2894:
URL: https://github.com/apache/tika/pull/2894#discussion_r3425421755
##########
tika-pipes/tika-pipes-plugins/tika-pipes-solr/src/main/java/org/apache/tika/pipes/emitter/solr/SolrEmitter.java:
##########
@@ -108,26 +109,41 @@ private static SolrClient
buildSolrClient(SolrEmitterConfig config) throws TikaC
if (config.solrUrls() == null || config.solrUrls().isEmpty()) {
// Use ZooKeeper-based CloudSolrClient
- Http2SolrClient.Builder http2SolrClientBuilder = new
Http2SolrClient.Builder();
- if (!StringUtils.isBlank(httpClientFactory.getUserName())) {
-
http2SolrClientBuilder.withBasicAuthCredentials(httpClientFactory.getUserName(),
httpClientFactory.getPassword());
- }
- http2SolrClientBuilder
+ HttpJettySolrClient.Builder jettyClientBuilder = new
HttpJettySolrClient.Builder();
+ applyAuthAndProxy(jettyClientBuilder, httpClientFactory,
config.proxyHost(), config.proxyPort());
+ jettyClientBuilder
.withRequestTimeout(httpClientFactory.getRequestTimeoutMillis(),
TimeUnit.MILLISECONDS)
.withConnectionTimeout(config.getConnectionTimeoutMillisOrDefault(),
TimeUnit.MILLISECONDS);
- Http2SolrClient http2SolrClient = http2SolrClientBuilder.build();
return new CloudSolrClient.Builder(config.solrZkHosts(),
Optional.ofNullable(config.solrZkChroot()))
- .withHttpClient(http2SolrClient)
+ .withInternalClientBuilder(jettyClientBuilder)
.build();
} else {
- // Use direct URL-based LBHttpSolrClient
- return new LBHttpSolrClient.Builder()
+ // Use direct URL-based LBJettySolrClient
+ HttpJettySolrClient.Builder jettyClientBuilder = new
HttpJettySolrClient.Builder();
+ applyAuthAndProxy(jettyClientBuilder, httpClientFactory,
config.proxyHost(), config.proxyPort());
+ jettyClientBuilder
.withConnectionTimeout(config.getConnectionTimeoutMillisOrDefault(),
TimeUnit.MILLISECONDS)
-
.withSocketTimeout(config.getSocketTimeoutMillisOrDefault(),
TimeUnit.MILLISECONDS)
- .withHttpClient(httpClientFactory.build())
- .withBaseEndpoints(config.solrUrls().toArray(new
String[]{}))
- .build();
+ .withIdleTimeout(config.getSocketTimeoutMillisOrDefault(),
TimeUnit.MILLISECONDS);
Review Comment:
In the direct-URL (LBJettySolrClient) code path, the Jetty client builder
doesn't set a request timeout, while the ZooKeeper (CloudSolrClient) path does.
This can change timeout behavior compared to the previous Apache
HttpClient-based implementation and may allow requests to hang longer than
expected.
##########
tika-pipes/tika-pipes-plugins/tika-pipes-solr/src/main/java/org/apache/tika/pipes/iterator/solr/SolrPipesIterator.java:
##########
@@ -180,27 +181,39 @@ private SolrClient createSolrClient() throws
TikaConfigException {
List<String> solrZkHosts = config.getSolrZkHosts() != null ?
config.getSolrZkHosts() : Collections.emptyList();
if (solrUrls.isEmpty()) {
- //TODO -- there's more that we need to pass through, including ssl
etc.
- Http2SolrClient.Builder http2SolrClientBuilder = new
Http2SolrClient.Builder();
- if (!StringUtils.isBlank(httpClientFactory.getUserName())) {
-
http2SolrClientBuilder.withBasicAuthCredentials(httpClientFactory.getUserName(),
httpClientFactory.getPassword());
- }
- http2SolrClientBuilder
+ HttpJettySolrClient.Builder jettyClientBuilder = new
HttpJettySolrClient.Builder();
+ applyAuthAndProxy(jettyClientBuilder, httpClientFactory);
+ jettyClientBuilder
.withRequestTimeout(httpClientFactory.getRequestTimeoutMillis(),
TimeUnit.MILLISECONDS)
.withConnectionTimeout(config.getConnectionTimeoutMillis(),
TimeUnit.MILLISECONDS);
-
- Http2SolrClient http2SolrClient = http2SolrClientBuilder.build();
return new CloudSolrClient.Builder(solrZkHosts,
Optional.ofNullable(config.getSolrZkChroot()))
- .withHttpClient(http2SolrClient)
+ .withInternalClientBuilder(jettyClientBuilder)
.build();
+ }
+ HttpJettySolrClient.Builder jettyClientBuilder = new
HttpJettySolrClient.Builder();
+ applyAuthAndProxy(jettyClientBuilder, httpClientFactory);
+ jettyClientBuilder
+ .withConnectionTimeout(config.getConnectionTimeoutMillis(),
TimeUnit.MILLISECONDS)
+ .withIdleTimeout(config.getSocketTimeoutMillis(),
TimeUnit.MILLISECONDS);
Review Comment:
In the direct-URL (LBJettySolrClient) code path, the Jetty client builder
doesn't set a request timeout, while the ZooKeeper (CloudSolrClient) path does.
For consistency (and to preserve HttpClientFactory's request timeout
semantics), consider applying the same request timeout here as well.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
