[
https://issues.apache.org/jira/browse/SOLR-15960?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17856955#comment-17856955
]
Jan Høydahl edited comment on SOLR-15960 at 6/22/24 6:52 PM:
-------------------------------------------------------------
Env vars is a perfect generic way to configure, well, environment stuff, plays
well with cloud native etc. I initiated this effort to get away from the
situation where introducing a new Java SysProp also needed an edit in bin/solr
and bin/solr.cmd.
I agree that most places in our code we should read the sysProp {{solr.foo}}
rather than the env {{{}SOLR_FOO{}}}. We of course need to test EnvUtils itself
so we know the envs are translated to sysprop, so we should not really need to
manipulate the {{ENV}} map in tests. Thus your plan to discourage the use of
{{getEnv*}} methods is probably sound.
However, I disagree that they should be removed or the class renamed. The class
does handle environment so EnvUtils is not wrong. We can add javadoc to
discourage use of ghe {{getEnv*}} methods when not strictly necessary, or we
could hide the methods behind some inner class, e.g.
{{{}EnvUtils.UseTheseIfYouCannotReadTheSystemProperty.getenv(){}}}.
I put in most of the {{getEnv()}} methods as convenience methods to access the
environment directly for when that is necessary, and the Int/Long/BoolList
methods with and without defaultValue makes other code less cluttered.
There are few reasons to read env.vars directly, the only place I know it is
needed is in {{OtelTracerConfigurator}} which uses OpenTelemetry's env auto
configuation.
*EDIT* - I had another look and I see that the Otel class in fact does not
really use EnvUtils, and all current uses of {{EnvUtils.getEnvXxx}} should
really instead use the properties. So I guess removing the {{getEnv}} methods
is warranted. And also add {{System.getenv}} to forbiddenAPIs so you need to
add an exception annotation to use it.
was (Author: janhoy):
Env vars is a perfect generic way to configure, well, environment stuff, plays
well with cloud native etc. I initiated this effort to get away from the
situation where introducing a new Java SysProp also needed an edit in bin/solr
and bin/solr.cmd.
I agree that most places in our code we should read the sysProp {{solr.foo}}
rather than the env {{{}SOLR_FOO{}}}. We of course need to test EnvUtils itself
so we know the envs are translated to sysprop, so we should not really need to
manipulate the {{ENV}} map in tests. Thus your plan to discourage the use of
{{getEnv*}} methods is probably sound.
However, I disagree that they should be removed or the class renamed. The class
does handle environment so EnvUtils is not wrong. We can add javadoc to
discourage use of ghe {{getEnv*}} methods when not strictly necessary, or we
could hide the methods behind some inner class, e.g.
{{{}EnvUtils.UseTheseIfYouCannotReadTheSystemProperty.getenv(){}}}.
I put in most of the {{getEnv()}} methods as convenience methods to access the
environment directly for when that is necessary, and the Int/Long/BoolList
methods with and without defaultValue makes other code less cluttered.
There are few reasons to read env.vars directly, the only place I know it is
needed is in {{OtelTracerConfigurator}} which uses OpenTelemetry's env auto
configuation. So we document that users can configure tracing with the same set
of {{OTEL_XXX}} environment variables that OTEL users also can use across other
clients such as python, JS etc. In the configurator we read the env to log what
is set, and override some settings.
> 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
