gerlowskija commented on code in PR #3876:
URL: https://github.com/apache/solr/pull/3876#discussion_r2573656857
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrHttpConstants.java:
##########
@@ -42,17 +42,11 @@ public interface SolrHttpConstants {
/** Maximum total connections allowed */
String PROP_MAX_CONNECTIONS = "maxConnections";
- /**
- * A Java system property to select the {@linkplain
HttpClientBuilderFactory} used for configuring
- * HTTP based SolrClients.
- */
- String SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY =
"solr.httpclient.builder.factory";
Review Comment:
Good idea, +1
##########
solr/bin/solr:
##########
@@ -342,7 +342,7 @@ fi
if [ -n "${SOLR_AUTH_TYPE:-}" ]; then
case "$(echo "$SOLR_AUTH_TYPE" | awk '{print tolower($0)}')" in
basic)
-
SOLR_AUTHENTICATION_CLIENT_BUILDER="org.apache.solr.client.solrj.impl.PreemptiveBasicAuthClientBuilderFactory"
+
SOLR_AUTHENTICATION_CLIENT_BUILDER="org.apache.solr.client.solrj.impl.PreemptiveBasicAuthClientCustomizer"
Review Comment:
+1, renaming this prop makes sense to me since there's no "builder" involved
here any more. Ditto for "AUTHC_CLIENT_BUILDER_ARG" below.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/PreemptiveBasicAuthClientCustomizer.java:
##########
@@ -66,15 +68,15 @@ public static void setDefaultSolrParams(SolrParams params) {
}
@Override
- public void close() throws IOException {}
-
- @Override
- public void setup(Http2SolrClient client) {
+ public void setup(SolrClient client) {
+ if (client instanceof Http2SolrClient == false) {
+ return;
+ }
Review Comment:
[-0] I agree that putting "jetty" in the sysprop helps somewhat to
disambiguate the client type being targeted. Though that's really not my
primary concern.
My main interest here is around the Java API / signature itself. It feels
to me like the type-widening results in a Java API that's easier for a SolrJ
user to misuse, and harder for a Solr developer to implement than it was
before.
I *think* we're doing this in order to make the interface flexible enough to
customize JDK clients in the future? But if that's the goal, maybe we'd be
better off keeping this signature as-is for now, and instead adding a
`setup(HttpJdkSolrClient client)` down the road if/when we actually try to add
support for customizing those JDK clients.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
