HttpShardHandlerFactory urlScheme -- use sys prop for default #4008
Conversation
STCJ4 sets "urlScheme" and we should read it in HttpShardHandlerFactory as the default. Don't assume a test solr.xml has this.
| import static org.apache.solr.common.cloud.ZkStateReader.HTTPS; | ||
| import static org.apache.solr.common.cloud.ZkStateReader.URL_SCHEME; |
There was a problem hiding this comment.
STCJ4 should not be referencing SolrCloud stuff. Those constants are for ZkStateReader cluster properties, not test "urlScheme" which has no constant. That they are the same is a coincidence.
There was a problem hiding this comment.
That is an unfortunant coincidence, and it would be great to rename them to be more explciit, it's just so easy to get them confused, if we can't outright eliminate this..
|
note: this is intended to fix recent test failures from the premature change to have many tests no longer use a solr.xml, which results in creating one from purely internal coded defaults, which don't consider this system property (among many other things as well). |
| NamedList<?> args = info.initArgs; | ||
| this.scheme = getParameter(args, INIT_URL_SCHEME, null, sb); | ||
| // note: the sys prop is only used in testing | ||
| this.scheme = getParameter(args, INIT_URL_SCHEME, System.getProperty(INIT_URL_SCHEME), sb); |
There was a problem hiding this comment.
I totally believe you that this works, but it also just feels like a code smell.. why do we have to have a special magic property that is used in testing that is set up in this random method!
There was a problem hiding this comment.
I sympathize -- I don't love non-test code consulting a test-only system property. I guess I shouldn't have even claimed this was test-only since --even our production solr.xml uses this property-- (edit -- false). Once solr.ssl.enabled is available, this code will change to look at that. At least that is well namespaced.
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
Please do open that JIRA that you want now so that it doesn't get lost! As far as this goes, while I see that it works, it does feel weird that we don't just properly configure a HttpshardHandlerFactory in a more natural way then a magic "test" hook.
3 participants
HttpShardHandlerFactory reads the urlScheme from configuration, defaulting to "http" if there is none. Most/all solr.xml's we have, including for tests, have this explicitly configured with
urlSchemesys prop resolution. I think the factory should read that sys prop as a default, at least as a small improvement.Ultimately, I'd like to see SOLR_SSL_ENABLED used within Solr and we default from that, as that is what the user sets. But I'll leave that for a proper JIRA issue.