[
https://issues.apache.org/jira/browse/SOLR-15960?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17856840#comment-17856840
]
David Smiley commented on SOLR-15960:
-------------------------------------
After seeing this in use, I see some problems:
* Solr tests don't (auto) reset env vars, thus a test exercising an env var
must remember to manually reset it via EnvUtils.
[Here|https://github.com/apache/solr/blob/2a56bfc2d8b48d1c93d683515569e2e97acf2077/solr/core/src/test/org/apache/solr/cli/PostToolTest.java#L118]
is a test that didn't (CC [~epugh]). Sys props are automatically reset; our
tests don't even have to remember to do so (nice!). Perhaps the solution is to
figure out how to auto reset it but I have another suggestion...
* [Some Solr code
|https://github.com/apache/solr/blob/cae69c7973303653cade8f9de7b96e26ccd0919e/solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java#L104]
refers to EnvUtils.getEnv... erroneously when getProperty was intended. (CC
[~houston] ). Maybe this bug was exacerbated by a duality in having two ways
to access a setting. Heck even my colleagues instinctively wanted to call a
getEnv method on EnvUtils when really we should not commonly do so.
* Mutability of EnvUtils.ENV without thread-safety (e.g. synchronization) is
questionable... risks of concurrent access
Maybe this is controversial but I propose that our code should basically
_never_ access env vars in lieu of always using getProperty. Rely on EnvUtils
to auto populate props from the env. EnvUtils should have no tempting "env"
methods on it; rename this class to SolrProps. If a test must truly test env
access, consider BATS or docker instead.
(CC'ing two people only for awareness; no need to create one-liner fixes; I'd
rather fix this all at once as I suggest if we agree)
> Unified use of system properties and environment variables
> ----------------------------------------------------------
>
> Key: SOLR-15960
> URL: https://issues.apache.org/jira/browse/SOLR-15960
> Project: Solr
> Issue Type: Improvement
> Reporter: Jan Høydahl
> Assignee: Jan Høydahl
> Priority: Major
> Fix For: 9.5
>
> Time Spent: 10h 20m
> Remaining Estimate: 0h
>
> We have a lot of boiler-plate code in Solr related to resolving configuration
> from config-files, system properties and environment variables.
> The main pattern so far has been to load a config from an xml file, which
> uses system property variables like {{{}$\{myVar{}}}}. All the environment
> variables that we expose in {{solr.in.sh}} are converted to system.properties
> in {{bin/solr}} and inside of Solr we only care about sys.props. This has
> served us quite well, but is also has certain disadvantages:
> * Naming mismatches. You have one config name in the xml file, one as system
> property and yet another for environment variable.
> * Duplicate code to deal with type conversion, and converting comma
> separated lists from env.var into Java lists
> * Every new config option needs to touch {{{}bin/solr{}}}, {{bin/solr.cmd}}
> and often more.
> In the world of containers and k8s, we want to configure almost every aspect
> of an app using environment variables. It is sometimes also more secure than
> passing sys.props on the cmdline since they won't show up in a "ps".
> So this is a proposal to unify all Solr's configs in a more structured way
> * Make naming a convention. All env.variable should be uppercase with format
> {{SOLR_X_Y}} and all sys.propos should be lowercase with the format
> {{solr.x.y}}. Perhaps {{solr.camelCase}} should map to {{SOLR_CAMEL_CASE}},
> or we discourage camel case in favour of dots.
> * Add a central {{ConfigResolver}} class to Solr that can answer e.g.
> {{getInt("solr.my.number")}} and it would return either prop
> {{solr.my.number}} or {{SOLR_MY_NUMBER}}. Similar for String, bool etc, and
> with fallback-values
> * List support, e.g. {{getListOfStrings("solr.modules")}} and it would
> return a {{List<String>}} from either {{solr.modules}} or {{SOLR_MODULES}},
> supporting comma-separated, custom separator and why not also json list
> format ["foo","bar"]?
> A pitfall of using environment variables directly is testing, since env.vars
> are immutable. I suggest we solve this by reading all {{SOLR_*}}
> env.variables on startup and inserting them into a static, mutable map
> somewhere which is the single source of truth for env.vars. Then we can ban
> the use of {{System.getenv()}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
