Migrate to dot seperate naming of environment variables #3312

epugh · 2025-04-07T09:24:57Z

https://issues.apache.org/jira/browse/SOLR-XXXXX

Description

Replace legacy property names with the modern default dot notation.

Solution

This started out of some exploration of how to merge the solr.in.sh and solr.in.cmd files, and branched off from there.

I found that if we replaced the legacy variable names (that are mapped in EnvToSyspropMappings.properties) then we could simplify some of the code.

A question, do we need a deprecation type warning if this change is 10x only? One idea I had is that instead or removing the mappings from EnvToSyspropMappings.properties, we could instead have two lists. And if you use the value from the deprecated list, then you get a loud warning in the logs, with the idea that it gets removed in 11?

Tests

Running existing tests. If we add the warning idea, then will need another test.

janhoy · 2025-04-08T10:54:38Z

Great initiative. Warning is a nice thing to add since it is easy in a centralized way, but not strictly required if this is a 10.0 PR and the prop change is documented.

dsmiley · 2025-04-09T00:44:05Z

It's not clear this is needed or matters. In the code I like seeing "solr.<module>.<name>" where the module and name are camel case. But I can get over it.

epugh · 2025-04-09T00:46:27Z

Great initiative. Warning is a nice thing to add since it is easy in a centralized way, but not strictly required if this is a 10.0 PR and the prop change is documented.

Humm, so in messing with this, I am seeing that the properties that I've removed only get set by bin/solr scripts, so maybe they are all internal to Solr?

epugh · 2025-04-09T08:25:42Z

It's not clear this is needed or matters. In the code I like seeing "solr.." wehre the module and name are camel case. But I can get over it.

Thanks for the honest feedback ;-). I'm happy to elaborate on reasons why if helpful ;-)

…ile.types Use EnvUtils to simplify consulting properties or env variables.

Converts non standard old system properties into the current standard and warns users about upgrading.

Adds a bats test to show the deprecated value still works, but emits a warning to the user.

HoustonPutman · 2025-04-09T16:19:38Z

Yeah, I'm not sure this is better (and definitely not sure if its worth the effort)

epugh · 2025-04-09T16:33:56Z

We should probably make sure we are all on the same page, before I keep plugging away on this. I, maybe erroneously, assumed that since there was a TODO in the https://github.com/apache/solr/blob/main/solr/solrj/src/resources/EnvToSyspropMappings.properties#L6 file that this was something as a community we had agreed on. Since Solr 10 is coming, I wanted to get this in! I may over value consistency!

epugh · 2025-04-09T21:06:01Z

Okay, before I polish off migrating the rest of the ones in https://github.com/apache/solr/pull/3312/files#diff-012f0766c431d2622a99ac1f70addc1d1f90106816a94434a39db6945db23601, I think this should probably get merged, and then I can do a second batch...?

github-actions · 2025-06-09T00:00:26Z

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

janhoy

In SIP-21 (https://cwiki.apache.org/confluence/display/SOLR/System+property+naming+structure) I proposed a solr.<top-cat>.<sub-cat>.<prop>.<name> structure to give users some clue as to which part of solr a variable is for. It was a bit hard to agree on sensible top- and sub-categories, but I gave it a first shot. I made some comments below for how that would change this PR.

Also the comment in the props file was made assuming the community agrees to go this direction. The feedback from the SIP process was largely positive, and I know that more recently named variables have tried to follow this pattern, but no enforcement until you proposed this PR, which I support.

janhoy · 2025-06-09T12:45:44Z

solr/core/src/java/org/apache/solr/core/CoreContainer.java

@@ -1766,7 +1766,7 @@ private SolrCore createFromDescriptor(
        // this mostly happens when the core is deleted when this node is down
        // but it can also happen if connecting to the wrong zookeeper
        final boolean deleteUnknownCores =
-            Boolean.parseBoolean(System.getProperty("solr.deleteUnknownCores", "false"));
+            Boolean.parseBoolean(System.getProperty("solr.delete.unknown.cores", "false"));


solr.cloud.startup.delete.unknown.cores.enabled?

I can see that this is a painfully long prop, and I kind of agree with Smiley that solr.cloud.startup.deleteUnknownCores would be a better choice. But allowing CamelCase breaks the simple translation rule that you can take an env.var and lowercase it and replace underscores with dots to get the sysprop. Perhaps we could introduce dashes like SOLR_CLOUD_STARTUP_DELETE-UNKNOWN-CORES but it soon gets to complex. Naming has been and is a hard thing.

I'm personally willing to embrace structure and simplified rules for parsing/mapping and sacrifice some readability or brewity. Perhaps further down the road we could build one very simple config parser that instantiates config objects without all the bolier-plate parsing code we have today.

janhoy · 2025-06-09T12:46:05Z

solr/core/src/java/org/apache/solr/cli/PackageTool.java

@@ -334,7 +334,7 @@ public String getHeader() {
        "Note: (a) Please add '--solr-url http://host:port' parameter if needed (usually on Windows).");
    format(
        sb,
-        "      (b) Please make sure that all Solr nodes are started with '-Denable.packages=true' parameter.");
+        "      (b) Please make sure that all Solr nodes are started with '-Dsolr.enable.packages=true' parameter.");


solr.packages.enabled ?

janhoy · 2025-06-09T12:47:33Z

solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java

  private static final boolean disabled =
-      Boolean.parseBoolean(System.getProperty("disableAdminUI", "false"));
+      Boolean.parseBoolean(System.getProperty("solr.admin.ui.disabled", "false"));


solr.ui.disabled ?

janhoy · 2025-06-09T12:48:19Z

solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java

-      enableRemoteStreams = Boolean.getBoolean("solr.enableRemoteStreaming");
-      enableStreamBody = Boolean.getBoolean("solr.enableStreamBody");
+      enableRemoteStreams = Boolean.getBoolean("solr.enable.remote.streaming");
+      enableStreamBody = Boolean.getBoolean("solr.enable.stream.body");


solr.requests.streaming.remote.enabled and solr.requests.streaming.body.enabled ?

janhoy · 2025-06-09T12:54:56Z

solr/solrj/src/java/org/apache/solr/common/util/EnvUtils.java

+          InputStream stream2 =
+              EnvUtils.class
+                  .getClassLoader()
+                  .getResourceAsStream("EnvToSyspropDeprecatedMappings.properties")) {


More meaninful variable names than stream and stream2.
The file name EnvToSyspropDeprecatedMappings.properties gives impression that this file maps env.variables to sys props, but the file itself says it maps sys props to sys props?

Q: Would it be possible to add deprecations as a section in the existing properties file, perhaps with a prefix deprecated.* to keep it in one place?

epugh · 2025-06-10T16:02:56Z

Thanks for letting oking at this! I had tuna but out of steam on it but will try and pick it up again and get this batch of changes in. Definitely 10x change.

epugh added 9 commits April 5, 2025 17:38

Simplify life by using EnvUtil to avoid extra steps.

c01f7a3

Clear out previous look up way

e5db437

EnvUtils has done it's magic, so use System.getProperty

d195102

Align on solr.default.confdir pattern everywhere.

1d3adfe

standardize on solr.delete.unknown.cores name

59b4d55

Move waitForZk property standard solr.wait.for.zk pattern

58e71dd

Use the standardized name everywhere.

42d9f1c

Standardize on the plural solr.logs.dir form everywhere.

3ea631c

comment language tweak

898350f

github-actions bot added documentation Improvements or additions to documentation docker Docker image jetty-server client:solrj tool:build start-scripts tests cat:search cat:cloud cat:cli scripts labels Apr 7, 2025

Migrate to correct property name.

064d9a0

epugh changed the title ~~Migrate to env util~~ Migrate to dot seperate naming of environment variables Apr 7, 2025

epugh requested a review from janhoy April 7, 2025 12:09

epugh added 3 commits April 9, 2025 05:59

Migrate solrConfigSetForbiddenFileTypes to solr.configset.forbidden.f…

5dc09bb

…ile.types Use EnvUtils to simplify consulting properties or env variables.

Update docs to use the migrated pattern.

16c941f

Support deprecation warning of system properties

ed1362e

Converts non standard old system properties into the current standard and warns users about upgrading.

epugh added 2 commits April 9, 2025 10:26

Clarify when you need a mapping.

b12e762

Migrate enable.package to solr.enable.package

e204ea7

Adds a bats test to show the deprecated value still works, but emits a warning to the user.

github-actions bot added the cat:packagemanager label Apr 9, 2025

epugh added 2 commits April 9, 2025 12:51

Format log message using best practices.

6946605

Remove the actual mapping!

6e55300

epugh marked this pull request as ready for review April 9, 2025 21:04

github-actions bot added the stale PR not updated in 60 days label Jun 9, 2025

janhoy reviewed Jun 9, 2025

View reviewed changes

github-actions bot removed the stale PR not updated in 60 days label Jun 10, 2025

Migrate to dot seperate naming of environment variables #3312

Are you sure you want to change the base?

Migrate to dot seperate naming of environment variables #3312

Uh oh!

Conversation

epugh commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Uh oh!

janhoy commented Apr 8, 2025

Uh oh!

dsmiley commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epugh commented Apr 9, 2025

Uh oh!

epugh commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HoustonPutman commented Apr 9, 2025

Uh oh!

epugh commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epugh commented Apr 9, 2025

Uh oh!

github-actions bot commented Jun 9, 2025

Uh oh!

janhoy left a comment

Choose a reason for hiding this comment

Uh oh!

janhoy Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

janhoy Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

janhoy Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

janhoy Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

janhoy Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

janhoy Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

epugh commented Jun 10, 2025

Uh oh!

Uh oh!

epugh commented Apr 7, 2025 •

edited

Loading

dsmiley commented Apr 9, 2025 •

edited

Loading

epugh commented Apr 9, 2025 •

edited

Loading

epugh commented Apr 9, 2025 •

edited

Loading