Re: [PR] Migrate to dot seperate naming of environment variables [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2959835025 Thanks for letting oking at this! I had tuna but out of steam on it but will try and pick it up again and get this batch of changes in. Definitely 10x change. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
janhoy commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2135688338
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -1766,7 +1766,7 @@ private SolrCore createFromDescriptor(
// this mostly happens when the core is deleted when this node is down
// but it can also happen if connecting to the wrong zookeeper
final boolean deleteUnknownCores =
-Boolean.parseBoolean(System.getProperty("solr.deleteUnknownCores",
"false"));
+
Boolean.parseBoolean(System.getProperty("solr.delete.unknown.cores", "false"));
Review Comment:
I can see that this is a painfully long prop, and I kind of agree with
Smiley that `solr.cloud.startup.deleteUnknownCores` would be a better choice.
But allowing CamelCase breaks the simple translation rule that you can take an
env.var and lowercase it and replace underscores with dots to get the sysprop.
Perhaps we could introduce dashes like
`SOLR_CLOUD_STARTUP_DELETE-UNKNOWN-CORES` but it soon gets to complex. Naming
has been and is a hard thing.
I'm personally willing to embrace structure and simplified rules for
parsing/mapping and sacrifice some readability or brewity. Perhaps further down
the road we could build one very simple config parser that instantiates config
objects without all the bolier-plate parsing code we have today.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
janhoy commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2135670969
##
solr/solrj/src/java/org/apache/solr/common/util/EnvUtils.java:
##
@@ -32,28 +33,47 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;
import org.apache.solr.common.SolrException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Provides convenient access to System Properties for Solr. It also converts
'SOLR_FOO' env vars to
* system properties 'solr.foo', which is done on first access of this class.
All Solr code should
* use this in lieu of JDK equivalents.
*/
public class EnvUtils {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
/** Maps ENV keys to sys prop keys for special/custom mappings */
private static final Map CUSTOM_MAPPINGS = new HashMap<>();
+ /** Maps deprecated sys prop keys to current sys prop keys with
special/custom mappings */
+ private static final Map DEPRECATED_MAPPINGS = new
HashMap<>();
+
private static final Map camelCaseToDotsMap = new
ConcurrentHashMap<>();
static {
try {
Properties props = new Properties();
+ Properties deprecatedProps = new Properties();
try (InputStream stream =
-
EnvUtils.class.getClassLoader().getResourceAsStream("EnvToSyspropMappings.properties"))
{
+ EnvUtils.class
+ .getClassLoader()
+ .getResourceAsStream("EnvToSyspropMappings.properties");
+ InputStream stream2 =
+ EnvUtils.class
+ .getClassLoader()
+
.getResourceAsStream("EnvToSyspropDeprecatedMappings.properties")) {
Review Comment:
More meaninful variable names than `stream` and `stream2`.
The file name `EnvToSyspropDeprecatedMappings.properties` gives impression
that this file maps env.variables to sys props, but the file itself says it
maps sys props to sys props?
Q: Would it be possible to add deprecations as a section in the existing
properties file, perhaps with a prefix `deprecated.*` to keep it in one place?
##
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##
@@ -1766,7 +1766,7 @@ private SolrCore createFromDescriptor(
// this mostly happens when the core is deleted when this node is down
// but it can also happen if connecting to the wrong zookeeper
final boolean deleteUnknownCores =
-Boolean.parseBoolean(System.getProperty("solr.deleteUnknownCores",
"false"));
+
Boolean.parseBoolean(System.getProperty("solr.delete.unknown.cores", "false"));
Review Comment:
`solr.cloud.startup.delete.unknown.cores.enabled`?
##
solr/core/src/java/org/apache/solr/cli/PackageTool.java:
##
@@ -334,7 +334,7 @@ public String getHeader() {
"Note: (a) Please add '--solr-url http://host:port' parameter if
needed (usually on Windows).");
format(
sb,
-" (b) Please make sure that all Solr nodes are started with
'-Denable.packages=true' parameter.");
+" (b) Please make sure that all Solr nodes are started with
'-Dsolr.enable.packages=true' parameter.");
Review Comment:
`solr.packages.enabled` ?
##
solr/core/src/java/org/apache/solr/servlet/SolrRequestParsers.java:
##
@@ -114,8 +114,8 @@ public SolrRequestParsers(SolrConfig globalConfig) {
formUploadLimitKB = globalConfig.getFormUploadLimitKB();
// security risks; disabled by default
- enableRemoteStreams = Boolean.getBoolean("solr.enableRemoteStreaming");
- enableStreamBody = Boolean.getBoolean("solr.enableStreamBody");
+ enableRemoteStreams = Boolean.getBoolean("solr.enable.remote.streaming");
+ enableStreamBody = Boolean.getBoolean("solr.enable.stream.body");
Review Comment:
`solr.requests.streaming.remote.enabled` and
`solr.requests.streaming.body.enabled` ?
##
solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java:
##
@@ -39,9 +39,9 @@
*/
public final class LoadAdminUiServlet extends HttpServlet {
- // check system properties for whether or not admin UI is disabled, default
is false
+ // check system properties for whether the admin UI is disabled, default is
false
private static final boolean disabled =
- Boolean.parseBoolean(System.getProperty("disableAdminUI", "false"));
+ Boolean.parseBoolean(System.getProperty("solr.admin.ui.disabled",
"false"));
Review Comment:
`solr.ui.disabled` ?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
--
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
github-actions[bot] commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2954342106 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the d...@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2790997149 Okay, before I polish off migrating the rest of the ones in https://github.com/apache/solr/pull/3312/files#diff-012f0766c431d2622a99ac1f70addc1d1f90106816a94434a39db6945db23601, I think this should probably get merged, and then I can do a second batch...? -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2790338206 We should probably make sure we are all on the same page, before I keep plugging before. I, maybe erroneously, assumed that since there was a TODO in the https://github.com/apache/solr/blob/main/solr/solrj/src/resources/EnvToSyspropMappings.properties#L6 file that this was something as a community we had agreed on. Since Solr 10 is coming, I wanted to get this in! -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
HoustonPutman commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2790267014 Yeah, I'm not sure this is better (and definitely not sure if its worth the effort) -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2788766932 > It's not clear this is needed or matters. In the code I like seeing "solr.." wehre the module and name are camel case. But I can get over it. Thanks for the feedback ;-). I'm happy to elaborate on reasons why if helpful ;-) -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2787957727 > Great initiative. Warning is a nice thing to add since it is easy in a centralized way, but not strictly required if this is a 10.0 PR and the prop change is documented. Humm, so in messing with this, I am seeing that the properties that I've removed only get set by bin/solr scripts, so maybe they are all internal to Solr? -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
dsmiley commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2787953667 It's not clear this is needed or matters. In the code I like seeing "solr.." wehre the module and name are camel case. But I can get over it. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Migrate to dot seperate naming of environment variables [solr]
janhoy commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-2786040141 Great initiative. Warning is a nice thing to add since it is easy in a centralized way, but not strictly required if this is a 10.0 PR and the prop change is documented. -- 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-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
