Migrate to dot seperate naming of environment variables
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.
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.
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.
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?
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 ;-)
Yeah, I'm not sure this is better (and definitely not sure if its worth the effort)
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!
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...?
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!
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.
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!
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 ....
Just added a JIRA. I would like to get this batch committed, before I do more.....
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.
@dsmiley I think I have addressed your comments or asked for follow up details. Getting excited to get this first batch in!
@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?
I think we got a nice bit of clean up in the bash scripts as well!
@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...?
@HoustonPutman so this ones for you: #7a701d9366a0e3e302fbf4be38adfe7bc7db0529. This I think actually does the backwards compatible the way we want.
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!)