-
Notifications
You must be signed in to change notification settings - Fork 791
SOLR-17994: SolrClientCustomizer rename #3876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | ||
| title: Rename HttpClientBuilderFactory to SolrClientCustomizer, and property solr.httpclient.builder.factory to solr.solrj.http.customizer | ||
| type: other # added, changed, fixed, deprecated, removed, dependency_update, security, other | ||
| authors: | ||
| - name: David Smiley | ||
| links: | ||
| - name: SOLR-17994 | ||
| url: https://issues.apache.org/jira/browse/SOLR-17994 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,13 +96,15 @@ NOTE: Properties marked with "!" indicate inverted meaning between old and new p | |
|
|
||
| |solr.security.allow.urls|solr.allowUrls||A comma seperated list of urls for reading from. | ||
|
|
||
| |solr.security.allow.urls.enabled|!solr.disable.allow.urls|false|If using an allow list of accessible urls is enabled. | ||
| |solr.security.allow.urls.enabled|!solr.disable.allow.urls|false|If using an allow list of accessible urls is enabled. | ||
|
|
||
|
|
||
| |solr.security.auth.plugin|authenticationPlugin||Specifies the authentication plugin to use. | ||
|
|
||
| |solr.solrj.cloud.max.stale.retries|cloudSolrClientMaxStaleRetries|5|Sets the maximum number of retries for stale connection attempts in SolrJ cloud client. | ||
|
|
||
| |solr.solrj.http.customizer|solr.httpclient.builder.factory||A class loaded to customize HttpJettySolrClient upon creation. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense |
||
|
|
||
| |solr.streamingexpressions.facet.tiered.enabled|solr.facet.stream.tiered|true|Controls whether tiered faceting is enabled for streaming expressions. | ||
|
|
||
| |solr.streamingexpressions.macros.enabled|StreamingExpressionMacros|false|Controls whether to expand URL parameters inside of the `expr` parameter. | ||
|
|
@@ -142,8 +144,8 @@ System properties can be set in several ways: | |
| 2. In `solr.in.sh` (Unix) or `solr.in.cmd` (Windows) using environment variables | ||
| 3. Through environment variables (with appropriate naming conventions) | ||
|
|
||
| Environment variables can also be used to set these properties. | ||
| You may find this useful in environments such as Docker. | ||
| Environment variables can also be used to set these properties. | ||
| You may find this useful in environments such as Docker. | ||
| Environment variables should be uppercase with dot notations equivalents, e.g. `SOLR_API_V2_ENABLED` for the property `solr.api.v2.enabled`. | ||
|
|
||
| == See Also | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,7 +83,7 @@ Users who previously relied on collection-specific URLs to avoid including the c | |
| This makes it clear that they pertain specifically to “JavaBin” rather than binary in general. | ||
|
|
||
| * The deprecated SolrClient implementations based on Apache HttpClient are removed from Solrj, thus the related dependencies are no longer present. | ||
| The system property `solr.httpclient.builder.factory` now only configures SolrClients using a Jetty based HttpClient. | ||
| The system property `solr.solrj.http.customizer` (formerly `solr.httpclient.builder.factory`) now only configures SolrClients using a Jetty based HttpClient. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We haven't done in ref guide any "formerly known" language out side of the Solr properties ref guide page for all the renamed props. |
||
|
|
||
| === SolrCloud Overseer | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,16 +14,21 @@ | |
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.solr.client.solrj.impl; | ||
|
|
||
| import java.io.Closeable; | ||
| package org.apache.solr.client.solrj; | ||
|
|
||
| /** | ||
| * A config hook for post-configuration of a {@linkplain Http2SolrClient} by its builder. | ||
| * A config hook for post-configuration of a {@linkplain SolrClient} by its builder. It is not | ||
| * supported by all builders. | ||
| * | ||
| * @see SolrHttpConstants#SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY | ||
| * @see #CLIENT_CUSTOMIZER_SYSPROP | ||
| * @lucene.experimental | ||
| */ | ||
| public interface HttpClientBuilderFactory extends Closeable { | ||
| default void setup(Http2SolrClient client) {} | ||
| public interface SolrClientCustomizer { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't need to be Closeable! |
||
| /** | ||
| * A Java system property to select the {@linkplain SolrClientCustomizer} used for configuring | ||
| * HTTP based SolrClients. | ||
| */ | ||
| String CLIENT_CUSTOMIZER_SYSPROP = "solr.solrj.http.customizer"; | ||
|
|
||
| void setup(SolrClient client); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No longer mandate a specific type of SolrClient |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ | |
| import org.apache.solr.client.api.util.SolrVersion; | ||
| import org.apache.solr.client.solrj.ResponseParser; | ||
| import org.apache.solr.client.solrj.SolrClient; | ||
| import org.apache.solr.client.solrj.SolrClientCustomizer; | ||
| import org.apache.solr.client.solrj.SolrClientFunction; | ||
| import org.apache.solr.client.solrj.SolrRequest; | ||
| import org.apache.solr.client.solrj.SolrResponse; | ||
|
|
@@ -55,6 +56,7 @@ | |
| import org.apache.solr.common.params.SolrParams; | ||
| import org.apache.solr.common.params.UpdateParams; | ||
| import org.apache.solr.common.util.ContentStream; | ||
| import org.apache.solr.common.util.EnvUtils; | ||
| import org.apache.solr.common.util.ExecutorUtil; | ||
| import org.apache.solr.common.util.NamedList; | ||
| import org.apache.solr.common.util.ObjectReleaseTracker; | ||
|
|
@@ -164,7 +166,7 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) { | |
| this.idleTimeoutMillis = builder.getIdleTimeoutMillis(); | ||
|
|
||
| try { | ||
| applyHttpClientBuilderFactory(); | ||
| applyClientCustomizer(); | ||
| } catch (RuntimeException e) { | ||
| try { | ||
| this.close(); | ||
|
|
@@ -185,16 +187,15 @@ private void initAuthStoreFromExistingClient(HttpClient httpClient) { | |
| this.authenticationStore = (AuthenticationStoreHolder) httpClient.getAuthenticationStore(); | ||
| } | ||
|
|
||
| private void applyHttpClientBuilderFactory() { | ||
| String factoryClassName = | ||
| System.getProperty(SolrHttpConstants.SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY); | ||
| private void applyClientCustomizer() { | ||
| String factoryClassName = EnvUtils.getProperty(SolrClientCustomizer.CLIENT_CUSTOMIZER_SYSPROP); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now using EnvUtils; should now work consistently with the env var pattern if someone wishes |
||
| if (factoryClassName != null) { | ||
| log.debug("Using Http Builder Factory: {}", factoryClassName); | ||
| HttpClientBuilderFactory factory; | ||
| log.debug("Using {}", factoryClassName); | ||
| SolrClientCustomizer factory; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO rename to |
||
| try { | ||
| factory = | ||
| Class.forName(factoryClassName) | ||
| .asSubclass(HttpClientBuilderFactory.class) | ||
| .asSubclass(SolrClientCustomizer.class) | ||
| .getDeclaredConstructor() | ||
| .newInstance(); | ||
| } catch (InstantiationException | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ | |
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Properties; | ||
| import org.apache.solr.client.solrj.SolrClient; | ||
| import org.apache.solr.client.solrj.SolrClientCustomizer; | ||
| import org.apache.solr.client.solrj.util.SolrBasicAuthentication; | ||
| import org.apache.solr.common.params.MapSolrParams; | ||
| import org.apache.solr.common.params.SolrParams; | ||
|
|
@@ -38,7 +40,7 @@ | |
| * HttpClientConfigurer implementation providing support for preemptive Http Basic authentication | ||
| * scheme. | ||
| */ | ||
| public class PreemptiveBasicAuthClientBuilderFactory implements HttpClientBuilderFactory { | ||
| public class PreemptiveBasicAuthClientCustomizer implements SolrClientCustomizer { | ||
| /** | ||
| * A system property used to specify a properties file containing default parameters used for | ||
| * creating an HTTP client. This is specifically useful for configuring the HTTP basic auth | ||
|
|
@@ -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; | ||
| } | ||
|
Comment on lines
+71
to
+74
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gerlowskija , responding to your feedback: The code here isn't really different than the behavior today -- if you set the sys prop, it only applies to Http2SolrClient (Jetty). If we start using this mechanism at not only the Http SolrClient level but also CloudSolrClient or whatever else that may or may not apply to this customizer, then it should skip it. But I get your concern. Perhaps the property should reflect the usage... so |
||
| final String basicAuthUser = | ||
| CREDENTIAL_RESOLVER.defaultParams.get(SolrHttpConstants.PROP_BASIC_AUTH_USER); | ||
| final String basicAuthPass = | ||
| CREDENTIAL_RESOLVER.defaultParams.get(SolrHttpConstants.PROP_BASIC_AUTH_PASS); | ||
| this.setup(client, basicAuthUser, basicAuthPass); | ||
| this.setup((Http2SolrClient) client, basicAuthUser, basicAuthPass); | ||
| } | ||
|
|
||
| public void setup(Http2SolrClient client, String basicAuthUser, String basicAuthPass) { | ||
|
|
@@ -98,10 +100,9 @@ static class CredentialsResolver { | |
|
|
||
| public CredentialsResolver() { | ||
| String credentials = | ||
| System.getProperty( | ||
| PreemptiveBasicAuthClientBuilderFactory.SYS_PROP_BASIC_AUTH_CREDENTIALS); | ||
| System.getProperty(PreemptiveBasicAuthClientCustomizer.SYS_PROP_BASIC_AUTH_CREDENTIALS); | ||
| String configFile = | ||
| System.getProperty(PreemptiveBasicAuthClientBuilderFactory.SYS_PROP_HTTP_CLIENT_CONFIG); | ||
| System.getProperty(PreemptiveBasicAuthClientCustomizer.SYS_PROP_HTTP_CLIENT_CONFIG); | ||
|
|
||
| if (credentials != null && configFile != null) { | ||
| throw new IllegalArgumentException( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,12 +42,6 @@ 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"; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I felt this was better organized on the class that loads it. I can change back if asked. |
||
|
|
||
| /** | ||
| * System property consulted to determine if HTTP based SolrClients will require hostname | ||
| * validation of SSL Certificates. The default behavior is to enforce peer name validation. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this env var should be renamed too