SOLR-17994: SolrClientCustomizer rename #3876
Conversation
Rename HttpClientBuilderFactory to SolrClientCustomizer, and property solr.httpclient.builder.factory to solr.solrj.http.customizer
github-actions
bot
added
documentation
| 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" |
There was a problem hiding this comment.
perhaps this env var should be renamed too
| |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.cookies.enabled|!solr.http.disableCookies| false |If Http2SolrClient should support cookies. | ||
| |solr.solrj.http.customizer|solr.httpclient.builder.factory||A class loaded to customize HttpJettySolrClient upon creation. |
There was a problem hiding this comment.
HttpJettySolrClient slipped in here... hasn't been renamed from Http2SolrClient yet
| String factoryClassName = | ||
| System.getProperty(SolrHttpConstants.SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY); | ||
| private void applyClientCustomizer() { | ||
| String factoryClassName = EnvUtils.getProperty(SolrClientCustomizer.CLIENT_CUSTOMIZER_SYSPROP); |
There was a problem hiding this comment.
now using EnvUtils; should now work consistently with the env var pattern if someone wishes
| log.debug("Using Http Builder Factory: {}", factoryClassName); | ||
| HttpClientBuilderFactory factory; | ||
| log.debug("Using {}", factoryClassName); | ||
| SolrClientCustomizer factory; |
There was a problem hiding this comment.
TODO rename to instance
| * 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"; |
There was a problem hiding this comment.
I felt this was better organized on the class that loads it. I can change back if asked.
| * @see #CLIENT_CUSTOMIZER_SYSPROP | ||
| * @lucene.experimental | ||
| */ | ||
| public interface SolrClientCustomizer { |
There was a problem hiding this comment.
doesn't need to be Closeable!
| */ | ||
| String CLIENT_CUSTOMIZER_SYSPROP = "solr.solrj.http.customizer"; | ||
|
|
||
| void setup(SolrClient client); |
There was a problem hiding this comment.
No longer mandate a specific type of SolrClient
| public void setup(SolrClient client) { | ||
| if (client instanceof Http2SolrClient == false) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
@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 solr.solrj.http.jetty.customizer vs solr.solrj.http.jdk.customizer (hypothetical). With this approach, the implementation should throw an exception if the type is wrong. And the implementation should reflect it as well, like putting "Jetty" into PreemptiveBasicAuthClientJettyCustomizer. LMK what you think; I'd prefer this way.
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
I focused on the sys variable changes and they Lgtm.
|
|
||
| * 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. |
There was a problem hiding this comment.
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.
|
@gerlowskija if you only have time to review one of my PRs... this is one where more review would be better. |
2 participants
Rename HttpClientBuilderFactory to SolrClientCustomizer, and property solr.httpclient.builder.factory to solr.solrj.http.customizer
https://issues.apache.org/jira/browse/SOLR-17994
This somewhat fits into the separation of a solrj-jetty.