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-15960 Cut over System.getProperty() to EnvUtils for modules #2193

Merged
merged 5 commits into from
Jan 23, 2024
Merged

SOLR-15960 Cut over System.getProperty() to EnvUtils for modules #2193

merged 5 commits into from
Jan 23, 2024

Conversation

janhoy
Copy link
Contributor

@janhoy janhoy commented Jan 12, 2024

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

This is the first in a series of PRs to cut over use of System.getProperty() to EnvUtils.getProp(). This PR does it for the modules. Followup PRs will target core.

I'm re-using same JIRA since it is not yet released, and this is part of the same effort.

@janhoy janhoy requested a review from epugh January 12, 2024 21:57
@janhoy janhoy force-pushed the SOLR-15960-cut-over-System.getProperty branch from 1d60d9d to e599f5c Compare January 12, 2024 21:59
epugh
epugh approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. I sort of wish it was the full getProperty.....

@janhoy
Copy link
Contributor Author

janhoy commented Jan 12, 2024

LGTM. I sort of wish it was the full getProperty.....

We can rename getProp -> getProperty if you feel it is better. The code has still not left the building in a release...

@epugh
Copy link
Contributor

epugh commented Jan 12, 2024

LGTM. I sort of wish it was the full getProperty.....

We can rename getProp -> getProperty if you feel it is better. The code has still not left the building in a release...

It matches the System.getProperty pattern, and just seems like one less thing to mentally parse...

@janhoy
Copy link
Contributor Author

janhoy commented Jan 12, 2024

LGTM. I sort of wish it was the full getProperty.....

We can rename getProp -> getProperty if you feel it is better. The code has still not left the building in a release...

It matches the System.getProperty pattern, and just seems like one less thing to mentally parse...

It has crossed my mind. Can be a followup to this..

PS: Do you think that EnvUtils is a covering name, or would NodeEnv or SolrEnv or similar be better?

@epugh
Copy link
Contributor

epugh commented Jan 12, 2024

LGTM. I sort of wish it was the full getProperty.....

We can rename getProp -> getProperty if you feel it is better. The code has still not left the building in a release...

It matches the System.getProperty pattern, and just seems like one less thing to mentally parse...

It has crossed my mind. Can be a followup to this..

PS: Do you think that EnvUtils is a covering name, or would NodeEnv or SolrEnv or similar be better?

I think EnvUtils is just fine. It lets you cover lots of environment related stuff!

@janhoy janhoy mentioned this pull request Jan 12, 2024
Merged
@epugh
Copy link
Contributor

epugh commented Jan 22, 2024

The latest commits are looking great.... Thanks for the rename!

@janhoy janhoy merged commit e2a333e into apache:main Jan 23, 2024
3 checks passed
@janhoy janhoy deleted the SOLR-15960-cut-over-System.getProperty branch January 23, 2024 12:01
janhoy added a commit to janhoy/solr that referenced this pull request Jan 23, 2024
@janhoy
SOLR-15960 Cut over System.getProperty() to EnvUtils for modules (apa…
0243f60 
…che#2193)
@janhoy janhoy mentioned this pull request Jan 23, 2024
Merged
freedev added a commit to freedev/solr that referenced this pull request Jan 26, 2024
@freedev
Merge branch 'main' into jira/solr-17112
fa9bfc7 
* main: (27 commits)
  Update protected-branches to include branch_9_5 (apache#2211)
  SOLR-16397: Tweak v2 'REQUESTSTATUS' API to be more REST-ful  (apache#2144)
  SOLR-17120: handle null value when merging partials (apache#2214)
  SOLR-17119: Fix exception swallowing in /cluster/plugins (apache#2202)
  SOLR-15960 Cut over System.getProperty() to EnvUtils for modules (apache#2193)
  Final fix for node problems (apache#2208)
  SOLR-16397: Fix warning in merge-indices docs
  Fix nodeSetup, use node distBaseUrl instead of registry (apache#2208)
  Add next minor version 9.6.0
  SOLR-17089: Upgrade Jersey to 3.1.5 (apache#2178)
  solr-ref-guide: fix typo in result-clustering.adoc (apache#2210)
  SOLR-17074: Fixed not correctly escaped quote in bin/solr script (apache#2200)
  SOLR-15960: Rename getProp as getProperty (apache#2194)
  Add npmRegistry for nodeSetup as well (apache#2208)
  Give NPM registry option for downloading node tools (apache#2208)
  SOLR-17116: Fix INSTALLSHARDDATA async reporting (apache#2188)
  SOLR-17066: Replace 'data store' term in code and docs (apache#2201)
  SOLR-17121: Fix SchemaCodecFactory to get PostingsFormat and DocValues from field. (apache#2206)
  Sync CHANGES for 9.4.1
  Add bugfix version 9.4.1
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epugh epugh epugh approved these changes

Assignees
No one assigned
Labels
client:solrj module:extraction module:hadoop-auth module:hdfs module:jwt-auth module:ltr module:opentelemetry module:s3-repository test-framework
Projects
None yet
Milestone
No milestone
2 participants
@janhoy @epugh