Add sysprop driven XML tags to default solr.xml

[gerlowskija]

The default solr.xml now has the necessary plumbing to obey the commonly used solr.sharedLib, solr.allowPaths, and enableMetrics system properties.

Resolves #635

[HoustonPutman]

This looks good to me, I'm very excited to use the SOLR_MODULES option in 9.0. But until then this is great!

One question. If the user doesn't provide solr.sharedLib, the xml section will start with ,. Is that ok?

[HoustonPutman]

Oh, might as well include the solr.port.advertise here as well, unless you wanted to do that in a separate PR. No worries either way.

[gerlowskija]

If the user doesn't provide solr.sharedLib, the xml section will start with ,.

Hmm, not quite sure what you mean. Are you talking about the GenerateAdditionalLibXMLPart change here?

If so, AFAICT, 'libList' will never be empty because it at least always has ${solr.sharedLib:}. So if the user doesn't use any backups or modules, the produced XML should look like:

<str name="sharedLib">${solr.sharedLib:}</str>

But, maybe I'm missing something @HoustonPutman ?

might as well include the solr.port.advertise here as well

Makes sense to add that in while we're here, sure. Will add shortly

[HoustonPutman]

If the user doesn't provide solr.sharedLib, the xml section will start with ,.

Hmm, not quite sure what you mean. Are you talking about the GenerateAdditionalLibXMLPart change here?

If so, AFAICT, 'libList' will never be empty because it at least always has ${solr.sharedLib:}. So if the user doesn't use any backups or modules, the produced XML should look like:

<str name="sharedLib">${solr.sharedLib:}</str>

But, maybe I'm missing something @HoustonPutman ?

So my thought was if ${solr.sharedLib:} equates to an empty string, which it will by default, that Solr wouldn't handle the , in the beginning very well. But I just tested it out, and it works fine! So no issue there 🙂

[gerlowskija]

Ah, yes - sorry, I was being dense. Had an epiphany about what you meant while I was afk getting dinner. I tested it as well - looks good on 8.11 and 9.3 both. So I think we're safe 😅

[gerlowskija]

might as well include the solr.port.advertise here as well

I've added the necessary changes to work solr.port.advertise into generated solr.xml files, though I think it'll need tweaked a bit.

The generated solr.xml was already set up to reference a different system property called hostPort. It looks like we made pretty regular use of that sysprop by default: even going so far as to error out of reconciliation whenever a custom solr.xml didn't include <int name="hostPort">${hostPort:80}</int>. This PR (right now) updates this all to use solr.port.advertise instead.

Still doing a bit of testing on this and thinking through the upgrade ramifications (little worried about what this change means for administrators with custom solr.xml files that want to upgrade their operator), but this should all be ready for review.

[HoustonPutman]

So we can actually just use an envVar, SOLR_PORT_ADVERTISE, instead of relying on SOLR_OPTS. Makes the code a small bit cleaner 🙂

But we can keep the hostPort in place and deprecate it (So we would be using -DhostPort and SOLR_PORT_ADVERTISE at the same time), to give people with a custom solr.xml a release to change their Solr.xml to use solr.port.advertise, like is used in the official solr.xml. As long as we document it in the upgrade notes, we should be good to go!

[gerlowskija]

Ah, right! I forgot bin/solr was set up to convert SOLR_PORT_ADVERTISE to a sysprop that solr.xml can take advantage of, cool. 👍

[gerlowskija]

Alright, should be ready for review again. We're now setting the SOLR_PORT_ADVERTISE env var instead of adding -Dsolr.port.advertise to solrOpts. The custom solr.xml validation now accepts both hostPort and solr.port.advertise as placeholders to give users a release or two to upgrade any custom solr.xml files.

[HoustonPutman]

One little suggestion for the upgrade notes, but otherwise it looks good to me!