Copilot commented on code in PR #2894:
URL: https://github.com/apache/tika/pull/2894#discussion_r3422686797
##########
tika-parent/pom.xml:
##########
@@ -441,13 +438,15 @@
<slf4j.version>2.0.18</slf4j.version>
<sis.version>1.6</sis.version>
<snappy.version>1.1.10.8</snappy.version>
- <!-- TODO (incomplete) if updating to solrj 10:
-
https://solr.apache.org/guide/solr/latest/upgrade-notes/major-changes-in-solr-10.html
- add solr-solrj-jetty artifact, add log4j
- Http2SolrClient -> HttpJettySolrClient
- LBHttpSolrClient -> LBHttp2SolrClient -> LBJettySolrClient (doesn't work)
- -->
- <solrj.version>9.10.1</solrj.version>
+ <!-- Upgraded to 10.0.0 (from 9.10.1): SolrJ 9.x bundles Jetty 11 HTTP
client APIs
+ internally (Http2SolrClient / solr-solrj-jetty), which conflict with
Jetty 12.
+ SolrJ 10 renames Http2SolrClient -> HttpJettySolrClient (in
solr-solrj-jetty)
+ and LBHttpSolrClient -> LBJettySolrClient; the solr-solrj-jetty
artifact must
+ be added alongside solr-solrj wherever those classes are used.
+ NOTE: LBJettySolrClient no longer accepts an external Apache
HttpClient via
+ withHttpClient(); proxy/auth config via HttpClientFactory needs
rework in a
+ follow-up. -->
Review Comment:
The NOTE under the SolrJ 10 upgrade is now misleading: this PR wires basic
auth and proxy settings via HttpJettySolrClient.Builder, so it no longer "needs
rework in a follow-up" in the way the comment suggests. Please update the
comment to reflect the current behavior/limitations (e.g., only basic auth is
supported).
##########
tika-pipes/tika-pipes-plugins/tika-pipes-solr/src/main/java/org/apache/tika/pipes/emitter/solr/SolrEmitter.java:
##########
@@ -108,26 +109,45 @@ 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();
+ HttpJettySolrClient.Builder jettyClientBuilder = new
HttpJettySolrClient.Builder();
if (!StringUtils.isBlank(httpClientFactory.getUserName())) {
-
http2SolrClientBuilder.withBasicAuthCredentials(httpClientFactory.getUserName(),
httpClientFactory.getPassword());
+ if
(!"basic".equalsIgnoreCase(httpClientFactory.getAuthScheme())) {
+ throw new TikaConfigException("Only 'basic' auth scheme is
supported by HttpJettySolrClient; got: '"
+ + httpClientFactory.getAuthScheme() + "'");
Review Comment:
The auth/proxy configuration logic (including the authScheme validation and
error message) is duplicated in both branches (CloudSolrClient vs
LBJettySolrClient). This increases the chance of the two branches drifting over
time; consider extracting a small helper that applies auth/proxy options to a
HttpJettySolrClient.Builder.
##########
tika-pipes/tika-pipes-plugins/tika-pipes-solr/src/main/java/org/apache/tika/pipes/iterator/solr/SolrPipesIterator.java:
##########
@@ -180,27 +181,45 @@ 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();
+ HttpJettySolrClient.Builder jettyClientBuilder = new
HttpJettySolrClient.Builder();
if (!StringUtils.isBlank(httpClientFactory.getUserName())) {
-
http2SolrClientBuilder.withBasicAuthCredentials(httpClientFactory.getUserName(),
httpClientFactory.getPassword());
+ if
(!"basic".equalsIgnoreCase(httpClientFactory.getAuthScheme())) {
+ throw new TikaConfigException("Only 'basic' auth scheme is
supported by HttpJettySolrClient; got: '"
+ + httpClientFactory.getAuthScheme() + "'");
Review Comment:
The auth/proxy configuration and authScheme validation are duplicated for
the zk-based and URL-based clients. Consider validating authScheme once and
extracting common HttpJettySolrClient.Builder setup to a helper to avoid
divergence between the two branches.
--
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]
