Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh merged PR #3312: URL: https://github.com/apache/solr/pull/3312 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3213981429 I tweaked the `DeprecatedSystemPropertiesMappings.properties` file to add in the various changed mappings, which gives us back compat between 10 and 9. I also highlihgted in the file which should be removed in Solr 11, and put a place holder for Solr 12... Going to run the tests, and assuming all greeen (and or just a flaky test from crave ;-) ) going to merge today. @HoustonPutman thanks for the input on the meetup...(Also, nice thing about the virtual meetup is seeing folks faces and verbal tones helps me understand issues better than just text... A +1 or -1 is just a very binary piece of input!) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3211876134 @HoustonPutman so this ones for you: #7a701d9366a0e3e302fbf4be38adfe7bc7db0529. This I think actually does the backwards compatible the way we want. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2290538688
##
solr/bin/solr:
##
@@ -1016,10 +1016,6 @@ elif [[ $SOLR_HOME != /* ]]; then
fi
fi
-# Set the default configset dir to be bootstrapped as _default
-: "${SOLR_DEFAULT_CONFDIR:="$SOLR_SERVER_DIR/solr/configsets/_default/conf"}"
Review Comment:
Turns out that this exact variable declaration is ALSO defined in Java code
via ConfigSetService.getDefaultConfigDirPath.
I didn't go exhaustively thorugh all the places we might try to get a
default configset dir, relying on the tests... I have seen other places where
we declare stuff in the solr/bin files, but then redeclare them in Java code,
so I wasn't totally surprised.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2289716779
##
solr/bin/solr:
##
@@ -1016,10 +1016,6 @@ elif [[ $SOLR_HOME != /* ]]; then
fi
fi
-# Set the default configset dir to be bootstrapped as _default
-: "${SOLR_DEFAULT_CONFDIR:="$SOLR_SERVER_DIR/solr/configsets/_default/conf"}"
Review Comment:
how were you able to remove this altogether? I suppose that most SolrCloud
BATS integration test is likely to fail if we don't bootstrap this. So if they
pass, we didn't need this line here after all.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3208633972 @HoustonPutman I'd love to collaborate on a solid "backwards compatible" test that makes you comfortable about these changes... What are your thoughts on a strategy? I'd like to land this PR next week... so...? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3208632384 I think we got a nice bit of clean up in the bash scripts as well! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2288115633
##
solr/bin/solr:
##
@@ -114,7 +114,7 @@ fi
# Export variables we want to make visible to Solr sub-process
for var in $(compgen -v); do
- if [[ "$var" =~
^(SOLR_.*|DEFAULT_CONFDIR|ZK_.*|GCS_BUCKET|GCS_.*|S3_.*|OTEL_.*|AWS_.*)$ ]];
then
+ if [[ "$var" =~ ^(SOLR_.*|ZK_.*|GCS_BUCKET|GCS_.*|S3_.*|OTEL_.*|AWS_.*)$ ]];
then
Review Comment:
You probably need to restore this if we're deferring the confdir matter
##
solr/bin/solr:
##
@@ -1327,7 +1305,7 @@ function start_solr() {
# OOME is thrown. Program operation after OOME is unpredictable.
"-XX:+CrashOnOutOfMemoryError"
"-XX:ErrorFile=${SOLR_LOGS_DIR}/jvm_crash_%p.log" \
"-Djetty.home=$SOLR_SERVER_DIR" "-Dsolr.solr.home=$SOLR_HOME"
"-Dsolr.install.dir=$SOLR_TIP" "-Dsolr.install.symDir=$SOLR_TIP_SYM" \
-"-Dsolr.default.confdir=$DEFAULT_CONFDIR" "${LOG4J_CONFIG[@]}"
"${SCRIPT_SOLR_OPTS[@]}" "${SECURITY_MANAGER_OPTS[@]}" "${SOLR_ADMIN_UI}"
"${SOLR_OPTS[@]}")
Review Comment:
You probably need to restore this if we're deferring the confdir matter
##
solr/bin/solr:
##
@@ -1025,7 +1017,8 @@ elif [[ $SOLR_HOME != /* ]]; then
fi
# Set the default configset dir to be bootstrapped as _default
-: "${DEFAULT_CONFDIR:="$SOLR_SERVER_DIR/solr/configsets/_default/conf"}"
+: "${SOLR_DEFAULT_CONFDIR:="$SOLR_SERVER_DIR/solr/configsets/_default/conf"}"
+export SOLR_DEFAULT_CONFDIR
Review Comment:
You probably need to restore this if we're deferring the confdir matter
##
solr/bin/solr.cmd:
##
@@ -1144,23 +1120,23 @@ IF NOT EXIST "%SOLR_SERVER_DIR%\tmp" (
mkdir "%SOLR_SERVER_DIR%\tmp"
)
-IF "%DEFAULT_CONFDIR%"=="" set
"DEFAULT_CONFDIR=%SOLR_SERVER_DIR%\solr\configsets\_default\conf"
+IF "%SOLR_DEFAULT_CONFDIR%"=="" set
"SOLR_DEFAULT_CONFDIR=%SOLR_SERVER_DIR%\solr\configsets\_default\conf"
IF "%FG%"=="1" (
REM run solr in the foreground
title "Solr-%SOLR_PORT%"
echo %SOLR_PORT%>"%SOLR_TIP%"\bin\solr-%SOLR_PORT%.port
"%JAVA%" %SERVEROPT% %SOLR_JAVA_MEM% %START_OPTS% ^
-Dlog4j.configurationFile="%LOG4J_CONFIG%" -DSTOP.PORT=!STOP_PORT!
-DSTOP.KEY=%STOP_KEY% ^
--Dsolr.solr.home="%SOLR_HOME%" -Dsolr.install.dir="%SOLR_TIP%"
-Dsolr.install.symDir="%SOLR_TIP%" -Dsolr.default.confdir="%DEFAULT_CONFDIR%" ^
+-Dsolr.solr.home="%SOLR_HOME%" -Dsolr.install.dir="%SOLR_TIP%"
-Dsolr.install.symDir="%SOLR_TIP%" ^
-Djetty.port=%SOLR_PORT% -Djetty.home="%SOLR_SERVER_DIR%" ^
-Djava.io.tmpdir="%SOLR_SERVER_DIR%\tmp" -jar start.jar
%SOLR_JETTY_CONFIG% "%SOLR_JETTY_ADDL_CONFIG%"
) ELSE (
START /B "Solr-%SOLR_PORT%" /D "%SOLR_SERVER_DIR%" ^
"%JAVA%" %SERVEROPT% %SOLR_JAVA_MEM% %START_OPTS% ^
-Dlog4j.configurationFile="%LOG4J_CONFIG%" -DSTOP.PORT=!STOP_PORT!
-DSTOP.KEY=%STOP_KEY% ^
-Dsolr.log.muteconsole ^
--Dsolr.solr.home="%SOLR_HOME%" -Dsolr.install.dir="%SOLR_TIP%"
-Dsolr.install.symDir="%SOLR_TIP%" -Dsolr.default.confdir="%DEFAULT_CONFDIR%" ^
Review Comment:
You probably need to restore this if we're deferring the confdir matter
##
solr/bin/solr.cmd:
##
@@ -1144,23 +1120,23 @@ IF NOT EXIST "%SOLR_SERVER_DIR%\tmp" (
mkdir "%SOLR_SERVER_DIR%\tmp"
)
-IF "%DEFAULT_CONFDIR%"=="" set
"DEFAULT_CONFDIR=%SOLR_SERVER_DIR%\solr\configsets\_default\conf"
+IF "%SOLR_DEFAULT_CONFDIR%"=="" set
"SOLR_DEFAULT_CONFDIR=%SOLR_SERVER_DIR%\solr\configsets\_default\conf"
IF "%FG%"=="1" (
REM run solr in the foreground
title "Solr-%SOLR_PORT%"
echo %SOLR_PORT%>"%SOLR_TIP%"\bin\solr-%SOLR_PORT%.port
"%JAVA%" %SERVEROPT% %SOLR_JAVA_MEM% %START_OPTS% ^
-Dlog4j.configurationFile="%LOG4J_CONFIG%" -DSTOP.PORT=!STOP_PORT!
-DSTOP.KEY=%STOP_KEY% ^
--Dsolr.solr.home="%SOLR_HOME%" -Dsolr.install.dir="%SOLR_TIP%"
-Dsolr.install.symDir="%SOLR_TIP%" -Dsolr.default.confdir="%DEFAULT_CONFDIR%" ^
Review Comment:
You probably need to restore this if we're deferring the confdir matter
##
solr/bin/solr.cmd:
##
@@ -1144,23 +1120,23 @@ IF NOT EXIST "%SOLR_SERVER_DIR%\tmp" (
mkdir "%SOLR_SERVER_DIR%\tmp"
)
-IF "%DEFAULT_CONFDIR%"=="" set
"DEFAULT_CONFDIR=%SOLR_SERVER_DIR%\solr\configsets\_default\conf"
+IF "%SOLR_DEFAULT_CONFDIR%"=="" set
"SOLR_DEFAULT_CONFDIR=%SOLR_SERVER_DIR%\solr\configsets\_default\conf"
Review Comment:
You probably need to restore this if we're deferring the confdir matter
##
solr/solrj/src/resources/EnvToSyspropMappings.properties:
##
@@ -5,29 +5,21 @@
# This way, env SOLR_FOO_BAR will also match property 'solr.foo.bar' without a
mapping in this file
# TODO: Deprecate non-standard sysprops and standardize on solr.foo.bar in
Solr 10
AWS_PROFILE=aws.profile
-DEFAULT_CONFDIR=solr.default.confdir
Review Comment:
`solr.configset.default.confdir` sounds good to me.
##
solr/bin/solr.cmd:
##
@@ -
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2288084762
##
solr/core/src/test/org/apache/solr/cloud/ZkFailoverTest.java:
##
@@ -47,7 +47,7 @@ public static void setupCluster() throws Exception {
@AfterClass
public static void resetWaitForZk() {
-System.setProperty("solr.wait.for.zk", "30");
+System.setProperty("solr.tests.cloud.wait.for.zk.seconds", "30");
Review Comment:
this is wrong now
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3206189319 @dsmiley okay, I think we are down to coming up with a good name for `SOLR_DEFAULT_CONFDIR`, i proposed two options. Or save it for antoher PR, and then can I get a LGTM assuming green build? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2288053682
##
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:
I went with the first part, making the file name clearer... merging both
formats into one file made that file look werid, and harder to name... because
it was doing two jobs.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2287981583
##
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##
@@ -248,7 +248,7 @@ public static NodeConfig loadNodeConfig(Path solrHome,
Properties nodeProperties
// ZkFailoverTest test case...
String zkHost = nodeProperties.getProperty(SolrXmlConfig.ZK_HOST);
if (StrUtils.isNotNullOrEmpty(zkHost)) {
- int startUpZkTimeOut = 1000 * Integer.getInteger("waitForZk", 0);
+ int startUpZkTimeOut = 1000 * Integer.getInteger("solr.wait.for.zk", 0);
Review Comment:
I do NOT see it in bin/solr... I did find this:
https://issues.apache.org/jira/browse/SOLR-14503 ticket. I guess I can imagine
that SOLR_WAIT_FOR_ZK was translated to `waitForZk`. I DO see it documented in
solr.in.* 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2288008085 ## solr/solrj/src/resources/EnvToSyspropMappings.properties: ## @@ -5,29 +5,21 @@ # This way, env SOLR_FOO_BAR will also match property 'solr.foo.bar' without a mapping in this file # TODO: Deprecate non-standard sysprops and standardize on solr.foo.bar in Solr 10 AWS_PROFILE=aws.profile -DEFAULT_CONFDIR=solr.default.confdir Review Comment: I didn't see a proposed name in the cwiki page for this, it's just listed as `solr.default.confdir`... What do you think about `solr.configset.default.confdir`? Or maybe even `solr.configset.default` with a name, and that figures out the specific location of the confdir under the covers, but not something a user would change? The key bit is what configset you are using, with the default one being `_default`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2287981583
##
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##
@@ -248,7 +248,7 @@ public static NodeConfig loadNodeConfig(Path solrHome,
Properties nodeProperties
// ZkFailoverTest test case...
String zkHost = nodeProperties.getProperty(SolrXmlConfig.ZK_HOST);
if (StrUtils.isNotNullOrEmpty(zkHost)) {
- int startUpZkTimeOut = 1000 * Integer.getInteger("waitForZk", 0);
+ int startUpZkTimeOut = 1000 * Integer.getInteger("solr.wait.for.zk", 0);
Review Comment:
I do NOT see it in bin/solr... I did find this:
https://issues.apache.org/jira/browse/SOLR-14503 ticket. I guess I can imagine
that SOLR_WAIT_FOR_ZK get's translated to solr.wait.for.zk. I *think* it
SHOULD be added to solr.in.sh and solr.in.cmd so that it's 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2287931346 ## solr/bin/solr.in.sh: ## @@ -248,14 +248,14 @@ #SOLR_SECURITY_MANAGER_ENABLED=true # This variable provides you with the option to disable the Admin UI. If you uncomment the variable below and -# change the value to true. The option is configured as a system property as defined in SOLR_START_OPTS in the start +# change the value to false. The option is configured as a system property as defined in SOLR_START_OPTS in the start # scripts. -# SOLR_ADMIN_UI_DISABLED=false +# SOLR_UI_ENABLED=false Review Comment: gotcha -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2287907678
##
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:
Need to mull on this more!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2286673347 ## solr/solrj/src/resources/EnvToSyspropMappings.properties: ## @@ -5,29 +5,21 @@ # This way, env SOLR_FOO_BAR will also match property 'solr.foo.bar' without a mapping in this file # TODO: Deprecate non-standard sysprops and standardize on solr.foo.bar in Solr 10 AWS_PROFILE=aws.profile -DEFAULT_CONFDIR=solr.default.confdir Review Comment: ah cool. But "SOLR_DEFAULT_CONFDIR" is poorly named since "DEFAULT" isn't a category. I suppose you are deferring such a matter to another PR and are now just focused on certain ones. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2286671327
##
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##
@@ -248,7 +248,7 @@ public static NodeConfig loadNodeConfig(Path solrHome,
Properties nodeProperties
// ZkFailoverTest test case...
String zkHost = nodeProperties.getProperty(SolrXmlConfig.ZK_HOST);
if (StrUtils.isNotNullOrEmpty(zkHost)) {
- int startUpZkTimeOut = 1000 * Integer.getInteger("waitForZk", 0);
+ int startUpZkTimeOut = 1000 * Integer.getInteger("solr.wait.for.zk", 0);
Review Comment:
This one is in bin/solr and I recall discussing this setting with colleagues
once. Let's not assume it's only for tests just because you only see a test
flip the value in the code. There's no telling what users do.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2286667041 ## solr/bin/solr.in.sh: ## @@ -248,14 +248,14 @@ #SOLR_SECURITY_MANAGER_ENABLED=true # This variable provides you with the option to disable the Admin UI. If you uncomment the variable below and -# change the value to true. The option is configured as a system property as defined in SOLR_START_OPTS in the start +# change the value to false. The option is configured as a system property as defined in SOLR_START_OPTS in the start # scripts. -# SOLR_ADMIN_UI_DISABLED=false +# SOLR_UI_ENABLED=false Review Comment: I don't mean that. I mean the value in the comment should be inverted itself since you flipped the ENABLED|DISABLED. The commented value is supposed to be the default value. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2286665913 ## dev-docs/ui/testing-and-deployment.adoc: ## @@ -127,8 +127,8 @@ more secure production configuration. Like other modules, the new UI is enabled with the jetty module parameter `--module=new-ui`. When using the Solr CLI, users can disable the new UI with -`SOLR_ADMIN_UI_EXPERIMENTAL_DISABLED=true` or by disabling the entire UI with -`SOLR_ADMIN_UI_DISABLED=true`. +`SOLR_ADMIN_UI_EXPERIMENTAL_ENABLED=false` or by disabling the entire UI with +`SOLR_UI_ENABLED=false`. Review Comment: I understand 2 parameters, but they should share the same category as they are both UI. Maybe the "experimental" one should be SOLR_UI_EXPERIMENTAL_ENABLED. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3200226879 @dsmiley I think I have addressed your comments or asked for follow up details. Getting excited to get this first batch 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2284835429 ## dev-docs/ui/testing-and-deployment.adoc: ## @@ -127,8 +127,8 @@ more secure production configuration. Like other modules, the new UI is enabled with the jetty module parameter `--module=new-ui`. When using the Solr CLI, users can disable the new UI with -`SOLR_ADMIN_UI_EXPERIMENTAL_DISABLED=true` or by disabling the entire UI with -`SOLR_ADMIN_UI_DISABLED=true`. +`SOLR_ADMIN_UI_EXPERIMENTAL_ENABLED=false` or by disabling the entire UI with +`SOLR_UI_ENABLED=false`. Review Comment: The difference is that you can opt out of the new Kotlin based "Solr UI" and keep the classic JS based "Admin UI", hence the two parameters. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2284846543 ## solr/solrj/src/resources/EnvToSyspropMappings.properties: ## @@ -5,29 +5,21 @@ # This way, env SOLR_FOO_BAR will also match property 'solr.foo.bar' without a mapping in this file # TODO: Deprecate non-standard sysprops and standardize on solr.foo.bar in Solr 10 AWS_PROFILE=aws.profile -DEFAULT_CONFDIR=solr.default.confdir Review Comment: After digging through the start up scripts, I learned that we don't need this variable, it's ONLY ever used internally in the start up scripts. There is a SOLR_DEFAULT_CONFDIR that is now used, which follows the pattern, which means we don't need it in this file. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3200190170 > The upgrade guide ought to list old/deprecated properties, right? Or if not then, if I'm seeing this code correctly, we log a warning if one is used? Correct. There is a bats test for that. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312: URL: https://github.com/apache/solr/pull/3312#discussion_r2284827259 ## solr/bin/solr.in.sh: ## @@ -248,14 +248,14 @@ #SOLR_SECURITY_MANAGER_ENABLED=true # This variable provides you with the option to disable the Admin UI. If you uncomment the variable below and -# change the value to true. The option is configured as a system property as defined in SOLR_START_OPTS in the start +# change the value to false. The option is configured as a system property as defined in SOLR_START_OPTS in the start # scripts. -# SOLR_ADMIN_UI_DISABLED=false +# SOLR_UI_ENABLED=false Review Comment: do you mean you prefer the _DISABLED versus teh _ENABLED naming? AS in `SOLR_UI_DISABLED=true`? I was thinking that instead of a mishmash of _ENABLED and _DISABLED, we would always end in _ENABLED and they you would specifiy a true or false... -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2284816010
##
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##
@@ -248,7 +248,7 @@ public static NodeConfig loadNodeConfig(Path solrHome,
Properties nodeProperties
// ZkFailoverTest test case...
String zkHost = nodeProperties.getProperty(SolrXmlConfig.ZK_HOST);
if (StrUtils.isNotNullOrEmpty(zkHost)) {
- int startUpZkTimeOut = 1000 * Integer.getInteger("waitForZk", 0);
+ int startUpZkTimeOut = 1000 * Integer.getInteger("solr.wait.for.zk", 0);
Review Comment:
so, this is only used in by a test, so it appears the pattern from cwiki
page would be solr.tests.wait.for.zk.seconds...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
dsmiley commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2283261216
##
solr/bin/solr.in.sh:
##
@@ -248,14 +248,14 @@
#SOLR_SECURITY_MANAGER_ENABLED=true
# This variable provides you with the option to disable the Admin UI. If you
uncomment the variable below and
-# change the value to true. The option is configured as a system property as
defined in SOLR_START_OPTS in the start
+# change the value to false. The option is configured as a system property as
defined in SOLR_START_OPTS in the start
# scripts.
-# SOLR_ADMIN_UI_DISABLED=false
+# SOLR_UI_ENABLED=false
Review Comment:
flip default
##
solr/bin/solr.in.cmd:
##
@@ -224,12 +224,12 @@ REM set SOLR_SECURITY_MANAGER_ENABLED=true
REM This variable provides you with the option to disable the Admin UI. if you
uncomment the variable below and
REM change the value to true. The option is configured as a system property as
defined in SOLR_START_OPTS in the start
REM scripts.
-REM set SOLR_ADMIN_UI_DISABLED=false
+REM set SOLR_UI_ENABLED=false
Review Comment:
flip the default
##
solr/solrj/src/resources/EnvToSyspropMappings.properties:
##
@@ -5,29 +5,21 @@
# This way, env SOLR_FOO_BAR will also match property 'solr.foo.bar' without a
mapping in this file
# TODO: Deprecate non-standard sysprops and standardize on solr.foo.bar in
Solr 10
AWS_PROFILE=aws.profile
-DEFAULT_CONFDIR=solr.default.confdir
Review Comment:
I'm not seeing what you replaced that with.
##
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##
@@ -248,7 +248,7 @@ public static NodeConfig loadNodeConfig(Path solrHome,
Properties nodeProperties
// ZkFailoverTest test case...
String zkHost = nodeProperties.getProperty(SolrXmlConfig.ZK_HOST);
if (StrUtils.isNotNullOrEmpty(zkHost)) {
- int startUpZkTimeOut = 1000 * Integer.getInteger("waitForZk", 0);
+ int startUpZkTimeOut = 1000 * Integer.getInteger("solr.wait.for.zk", 0);
Review Comment:
no; it misses a category. "solr.cloud.wait.for.zk.seconds" (I'm proposing a
time unit while we're at it)
##
dev-docs/ui/testing-and-deployment.adoc:
##
@@ -127,8 +127,8 @@ more secure production configuration.
Like other modules, the new UI is enabled with the jetty module parameter
`--module=new-ui`.
When using the Solr CLI, users can disable the new UI with
-`SOLR_ADMIN_UI_EXPERIMENTAL_DISABLED=true` or by disabling the entire UI with
-`SOLR_ADMIN_UI_DISABLED=true`.
+`SOLR_ADMIN_UI_EXPERIMENTAL_ENABLED=false` or by disabling the entire UI with
+`SOLR_UI_ENABLED=false`.
Review Comment:
It seems wrong to *both* have "ADMIN_UI" and "UI". Pick one. Jan proposed
"UI" which I agree with.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2279513700
##
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:
i went ahead and made this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2279496792
##
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:
love 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2279491985
##
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:
what do you think about `solr.ui.enabled` and having it be defaulted to
true??
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2279490625
##
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:
Working on 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on code in PR #3312:
URL: https://github.com/apache/solr/pull/3312#discussion_r2279487564
##
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:
sounds good! I like that booleans are ending with `.enabled`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17864: Migrate to dot separated naming of environment variables for a initial set of variables. [solr]
epugh commented on PR #3312: URL: https://github.com/apache/solr/pull/3312#issuecomment-3192012317 Just added a JIRA. I would like to get this batch committed, before I do more. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
