-
Notifications
You must be signed in to change notification settings - Fork 790
SOLR-15442: Sync up Solr (Jetty) host and ports variable naming #3814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
solr.jetty.host ..> solr.host.bind and SOLR_HOST/host --> solr.host.advertise
|
|
||
| if [ -n "${SOLR_JETTY_HOST:-}" ]; then | ||
| SCRIPT_SOLR_OPTS+=("-Dsolr.jetty.host=$SOLR_JETTY_HOST") | ||
| if [ -n "${SOLR_HOST_BIND:-}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we look up solr.host.bind in java code with our envutils, then maybe we don't need to these lines, because the environment variable gets mapped to system property for us...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this isn't true because the variable is used in the jetty config files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right.... Jetty doesn't use our EnvUtils... And I guess chicken/egg.. We can't intercept it and set it...
|
I don't think i can do |
|
I took a stab at migrating to |
dsmiley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good; thanks.
changelog/unreleased/SOLR-15442.yml
Outdated
| @@ -0,0 +1,10 @@ | |||
| # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc | |||
| title: Sync up port and host variable naming. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users won't know what to think of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors Solr's port and host configuration properties to establish a clearer and more consistent naming convention. The changes introduce new property names that better distinguish between "bind" (the interface Solr listens on) and "advertise" (the interface Solr announces to clients/ZooKeeper), while maintaining backward compatibility through deprecation mappings.
- Renames
SOLR_PORT/jetty.porttoSOLR_PORT_LISTEN/solr.port.listen - Renames
SOLR_JETTY_HOST/solr.jetty.hosttoSOLR_HOST_BIND/solr.host.bind - Renames
SOLR_HOST/hosttoSOLR_HOST_ADVERTISE/solr.host.advertise - Updates all configuration files, scripts, documentation, and tests to use the new property names
Reviewed Changes
Copilot reviewed 19 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| solr/solrj/src/resources/EnvToSyspropMappings.properties | Updates environment variable to system property mapping for new SOLR_PORT_LISTEN |
| solr/solrj/src/resources/DeprecatedSystemPropertyMappings.properties | Adds deprecation mappings for old property names to new ones |
| solr/solrj/src/test/org/apache/solr/common/util/EnvUtilsTest.java | Updates test to use new SOLR_PORT_LISTEN variable |
| solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc | Documents the deprecations and new property names |
| solr/solr-ref-guide/modules/getting-started/pages/tutorial-aws.adoc | Updates documentation example to use SOLR_HOST_BIND |
| solr/solr-ref-guide/modules/deployment-guide/pages/securing-solr.adoc | Updates security documentation with new property names |
| solr/solr-ref-guide/modules/configuration-guide/pages/solr-properties.adoc | Adds documentation for new host and port properties |
| solr/solr-ref-guide/modules/configuration-guide/pages/configuring-solr-xml.adoc | Updates example to use solr.host.advertise |
| solr/server/solr/solr.xml | Updates default config to use solr.host.advertise |
| solr/server/etc/jetty-https.xml | Updates Jetty config to use solr.host.bind |
| solr/server/etc/jetty-http.xml | Updates Jetty config to use solr.host.bind |
| solr/modules/cross-dc/src/test-files/mirroring-solr.xml | Updates test config to use new advertise properties |
| solr/docker/templates/Dockerfile.body.template | Updates Docker default to use SOLR_HOST_BIND |
| solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java | Updates code to use EnvUtils.getProperty for solr.port.listen |
| solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java | Updates to use EnvUtils for solr.port.listen |
| solr/core/src/java/org/apache/solr/cli/SolrProcessManager.java | Updates process detection to check for both old and new port property |
| solr/core/src/java/org/apache/solr/cli/RunExampleTool.java | Updates to use SOLR_PORT_LISTEN environment variable |
| solr/core/src/java/org/apache/solr/cli/CLIUtils.java | Updates to use solr.port.listen property |
| solr/bin/solr.in.sh | Updates default environment variable names in comments and examples |
| solr/bin/solr.in.cmd | Updates default environment variable names in comments and examples |
| solr/bin/solr.cmd | Implements SOLR_PORT_LISTEN and SOLR_HOST_BIND with backward compatibility for deprecated variables |
| solr/bin/solr | Implements SOLR_PORT_LISTEN and SOLR_HOST_BIND with backward compatibility for deprecated variables |
| changelog/unreleased/SOLR-15442.yml | Adds changelog entry for the property sync |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
solr/solr-ref-guide/modules/deployment-guide/pages/securing-solr.adoc
Outdated
Show resolved
Hide resolved
…lr.adoc Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In some ways I don't love this visually, but it keeps the same variable as in the main solr scripts.
Co-authored-by: David Smiley <dsmiley@apache.org>
|
I need to manually test |
Interesting that docker tests passed even without this. I guess defaults...
…t a bit more visible.
Tested on a fresh box and it works!!!!!!!!! Tweaked ref guide cause that one property catches me ALL the time, and maybe I help the next person. |
|
Thanks for getting this done! |
|
will let jenkins tests run and then back port... |
Sync up port and host variable naming: * solr.jetty.host --> solr.host.bind * SOLR_HOST/host --> solr.host.advertise * jetty.port --> solr.port.listen and SOLR_PORT_LISTEN --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: David Smiley <dsmiley@apache.org>
Sync up port and host variable naming: * solr.jetty.host --> solr.host.bind * SOLR_HOST/host --> solr.host.advertise * jetty.port --> solr.port.listen and SOLR_PORT_LISTEN --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: David Smiley <dsmiley@apache.org>
https://issues.apache.org/jira/browse/SOLR-15442
solr.jetty.host ..> solr.host.bind and SOLR_HOST/host --> solr.host.advertise