You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2023/09/29 01:11:50 UTC
[GitHub] [solr-operator] gerlowskija opened a new pull request, #636: Add sysprop driven XML tags to default solr.xml
gerlowskija opened a new pull request, #636:
URL: https://github.com/apache/solr-operator/pull/636
The default solr.xml now has the necessary plumbing to obey the commonly used `solr.sharedLib`, `solr.allowPaths`, and `enableMetrics` system properties.
Resolves #635
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #636:
URL: https://github.com/apache/solr-operator/pull/636#issuecomment-1744002994
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 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #636:
URL: https://github.com/apache/solr-operator/pull/636
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #636:
URL: https://github.com/apache/solr-operator/pull/636#issuecomment-1743489917
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #636:
URL: https://github.com/apache/solr-operator/pull/636#discussion_r1344766826
##########
controllers/util/solr_util.go:
##########
@@ -134,7 +135,7 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
// Keep track of the SolrOpts that the Solr Operator needs to set
// These will be added to the SolrOpts given by the user.
- allSolrOpts := []string{"-DhostPort=$(SOLR_NODE_PORT)"}
Review Comment:
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!
##########
controllers/util/solr_util.go:
##########
@@ -134,7 +135,7 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
// Keep track of the SolrOpts that the Solr Operator needs to set
// These will be added to the SolrOpts given by the user.
- allSolrOpts := []string{"-DhostPort=$(SOLR_NODE_PORT)"}
Review Comment:
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #636:
URL: https://github.com/apache/solr-operator/pull/636#issuecomment-1746998625
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #636:
URL: https://github.com/apache/solr-operator/pull/636#discussion_r1345786066
##########
controllers/util/solr_util.go:
##########
@@ -134,7 +135,7 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
// Keep track of the SolrOpts that the Solr Operator needs to set
// These will be added to the SolrOpts given by the user.
- allSolrOpts := []string{"-DhostPort=$(SOLR_NODE_PORT)"}
Review Comment:
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. 👍
##########
controllers/util/solr_util.go:
##########
@@ -134,7 +135,7 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
// Keep track of the SolrOpts that the Solr Operator needs to set
// These will be added to the SolrOpts given by the user.
- allSolrOpts := []string{"-DhostPort=$(SOLR_NODE_PORT)"}
Review Comment:
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. 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #636:
URL: https://github.com/apache/solr-operator/pull/636#discussion_r1346172394
##########
docs/upgrade-notes.md:
##########
@@ -130,6 +130,8 @@ _Note that the Helm chart version does not contain a `v` prefix, which the downl
- The `POD_HOSTNAME` envVar in SolrCloud Pods has been deprecated. Use `POD_NAME` instead.
+- Use of the `hostPort` system property placeholder in custom solr.xml files has been deprecated. Use `solr.port.advertise` instead.
Review Comment:
```suggestion
- Use of the `hostPort` system property placeholder in custom solr.xml files has been deprecated. Use `<int name="hostPort">${solr.port.advertise:80}</int>`, the default value used by Solr, instead.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #636:
URL: https://github.com/apache/solr-operator/pull/636#issuecomment-1744089624
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #636:
URL: https://github.com/apache/solr-operator/pull/636#issuecomment-1743747944
> > 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](https://github.com/apache/solr-operator/pull/636/files#diff-653faf42eabedf3285e433f247c993282f035ee70781d151f8c8d68fee2621a3R858)?
>
> 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 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Add sysprop driven XML tags to default solr.xml [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #636:
URL: https://github.com/apache/solr-operator/pull/636#issuecomment-1743725754
> 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](https://github.com/apache/solr-operator/pull/636/files#diff-653faf42eabedf3285e433f247c993282f035ee70781d151f8c8d68fee2621a3R858)?
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 value produced XML should look like:
```
<str name="sharedLib">${solr.sharedLib:}</str>
```
But, maybe I'm missing something @HoustonPutman ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org