Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SOLR-17347: Remove ENV storage/retrieval in EnvUtils #2534

Merged
merged 3 commits into from
Jul 3, 2024
Merged

SOLR-17347: Remove ENV storage/retrieval in EnvUtils #2534

merged 3 commits into from
Jul 3, 2024

Conversation

dsmiley
Copy link
Contributor

@dsmiley dsmiley commented Jun 25, 2024

https://issues.apache.org/jira/browse/SOLR-17347

Given how new EnvUtils is, let's not worry about back-compat.

We could keep the name as-is; it's fine. Env utils are mapped to properties.

@dsmiley dsmiley requested a review from janhoy June 25, 2024 12:27
janhoy
janhoy reviewed Jun 25, 2024
View reviewed changes
Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but in general, lookup the property name solr.port and not the env.name SOLR_PORT. I don't think this will work at all, since we don't attempt any fallback or conversion in the getProperty methods.

solr/core/src/java/org/apache/solr/cli/SolrCLI.java Outdated Show resolved Hide resolved
solr/solrj/src/java/org/apache/solr/common/util/EnvUtils.java Outdated Show resolved Hide resolved
solr/solrj/src/test/org/apache/solr/common/util/EnvUtilsTest.java Outdated Show resolved Hide resolved
janhoy
janhoy approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Next step is to add System.getProperty* to ForbiddenAPIs?

@dsmiley dsmiley merged commit 07ff133 into apache:main Jul 3, 2024
4 checks passed
@dsmiley dsmiley deleted the removeEnvUtilsEnvMethods branch July 3, 2024 14:02
dsmiley added a commit that referenced this pull request Jul 3, 2024
@dsmiley
SOLR-17347: Remove ENV storage/retrieval in EnvUtils (#2534)
128ccfb 
Removed "env" methods because Solr code should almost always access global configuration via system properties.  Properties are augmented with the "env" nonetheless.

(cherry picked from commit 07ff133)
@dsmiley
Copy link
Contributor Author

dsmiley commented Jul 5, 2024

Next step is to add System.getProperty* to ForbiddenAPIs?

Go for it if you want. Lots of code to update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janhoy janhoy janhoy approved these changes

Assignees
No one assigned
Labels
cat:api cat:cli cat:cloud client:solrj module:opentelemetry test-framework tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dsmiley @janhoy