solr icon indicating copy to clipboard operation
solr copied to clipboard

Migrate to dot seperate naming of environment variables

Open epugh opened this issue 9 months ago • 9 comments

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.

epugh avatar Apr 07 '25 09:04 epugh

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.

janhoy avatar Apr 08 '25 10:04 janhoy

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.

dsmiley avatar Apr 09 '25 00:04 dsmiley

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 avatar Apr 09 '25 00:04 epugh

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 ;-)

epugh avatar Apr 09 '25 08:04 epugh

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

HoustonPutman avatar Apr 09 '25 16:04 HoustonPutman

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 avatar Apr 09 '25 16:04 epugh

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...?

epugh avatar Apr 09 '25 21:04 epugh

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!

github-actions[bot] avatar Jun 09 '25 00:06 github-actions[bot]

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 avatar Jun 10 '25 16:06 epugh

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!

github-actions[bot] avatar Aug 10 '25 00:08 github-actions[bot]

Just picking this up again and getting synced to latest... I am thinking this would only go to Solr 10....

Thanks for the comments @janhoy ....

epugh avatar Aug 15 '25 15:08 epugh

Just added a JIRA. I would like to get this batch committed, before I do more.....

epugh avatar Aug 15 '25 15:08 epugh

The upgrade guide ought to list old/deprecated properties, right? Or if not then, if I'm seeing this code correctly, we log a warning if one is used?

Correct. There is a bats test for that. I also added https://issues.apache.org/jira/browse/SOLR-17868 to make sure we don't forget to explicitly document the mappings.

epugh avatar Aug 19 '25 10:08 epugh

@dsmiley I think I have addressed your comments or asked for follow up details. Getting excited to get this first batch in!

epugh avatar Aug 19 '25 10:08 epugh

@dsmiley okay, I think we are down to coming up with a good name for SOLR_DEFAULT_CONFDIR, i proposed two options. Or save it for antoher PR, and then can I get a LGTM assuming green build?

epugh avatar Aug 20 '25 12:08 epugh

I think we got a nice bit of clean up in the bash scripts as well!

epugh avatar Aug 21 '25 01:08 epugh

@HoustonPutman I'd love to collaborate on a solid "backwards compatible" test that makes you comfortable about these changes... What are your thoughts on a strategy? I'd like to land this PR next week... so...?

epugh avatar Aug 21 '25 01:08 epugh

@HoustonPutman so this ones for you: #7a701d9366a0e3e302fbf4be38adfe7bc7db0529. This I think actually does the backwards compatible the way we want.

epugh avatar Aug 21 '25 19:08 epugh

I tweaked the DeprecatedSystemPropertiesMappings.properties file to add in the various changed mappings, which gives us back compat between 10 and 9..... I also highlihgted in the file which should be removed in Solr 11, and put a place holder for Solr 12...

Going to run the tests, and assuming all greeen (and or just a flaky test from crave ;-) ) going to merge today. @HoustonPutman thanks for the input on the meetup... (Also, nice thing about the virtual meetup is seeing folks faces and verbal tones helps me understand issues better than just text... A +1 or -1 is just a very binary piece of input!)

epugh avatar Aug 22 '25 11:08 epugh