Re: [PR] SOLR-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
epugh merged PR #3604: URL: https://github.com/apache/solr/pull/3604 -- 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
dsmiley commented on code in PR #3604: URL: https://github.com/apache/solr/pull/3604#discussion_r2319728252 ## solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc: ## @@ -47,7 +47,8 @@ To learn about the updated options in each CLI tool, use the `--help` option or Additionally, the `bin/solr delete` command no longer deletes a configset when you delete a collection. Previously if you deleted a collection, it would also delete it's associated configset if it was the only user of it. Now you have to explicitly provide a `--delete-config` option to delete the configsets. This decouples the lifecycle of a configset from that of a collection. -The `bin/solr start --bootstrap_conf` flag is a legacy feature for converting from Solr to SolrCloud mode and has been removed. +The system property "bootstrap_conf" (recently renamed to "solr.configset.bootstrap.confdir") used in `bin/solr start` allwowed a collection creation command to default to the examination of system properties in the absence of proper creation parameters by the same name (like "collection.configName"). Review Comment: I meant "bootstrap_confdir" here. -- 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
epugh commented on code in PR #3604: URL: https://github.com/apache/solr/pull/3604#discussion_r2318956151 ## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java: ## @@ -702,13 +700,6 @@ public static void createCollectionZkNode( } } -// if the config name wasn't passed in, use the default -if (!collectionProps.containsKey(ZkController.CONFIGNAME_PROP)) - collectionProps.put(ZkController.CONFIGNAME_PROP, defaultConfigName); Review Comment: Thanks for taking a look at this... I almost tagged those lines cause I too felt they had a smell around them. It just felt like some totally undocumented "magic" that was waiting to screw someone up! I will make that change and then with a green, merge this. -- 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
dsmiley commented on code in PR #3604:
URL: https://github.com/apache/solr/pull/3604#discussion_r2316953866
##
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java:
##
@@ -702,13 +700,6 @@ public static void createCollectionZkNode(
}
}
-// if the config name wasn't passed in, use the default
-if (!collectionProps.containsKey(ZkController.CONFIGNAME_PROP))
- collectionProps.put(ZkController.CONFIGNAME_PROP,
defaultConfigName);
-
- } else if (Boolean.getBoolean("bootstrap_conf")) {
Review Comment:
Glad to see this removed but should be mentioned in the upgrade notes
##
solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java:
##
@@ -702,13 +700,6 @@ public static void createCollectionZkNode(
}
}
-// if the config name wasn't passed in, use the default
-if (!collectionProps.containsKey(ZkController.CONFIGNAME_PROP))
- collectionProps.put(ZkController.CONFIGNAME_PROP,
defaultConfigName);
Review Comment:
you removed this logic, thus ignored the config name specified in the
request. Surely we want to continue to do 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
dsmiley commented on code in PR #3604: URL: https://github.com/apache/solr/pull/3604#discussion_r2317633770 ## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java: ## @@ -702,13 +700,6 @@ public static void createCollectionZkNode( } } -// if the config name wasn't passed in, use the default -if (!collectionProps.containsKey(ZkController.CONFIGNAME_PROP)) - collectionProps.put(ZkController.CONFIGNAME_PROP, defaultConfigName); Review Comment: I took a fresh look at createCollectionZkNode. There's definitely some legacy logic here that I'd love to see gone. Only 2 lines are for bootstrap_conf, which you removed (an else-if); thanks. I think the "bootstrap_conf" else-if block can entirely go as well (~18 lines)! IMO All we want this newly renamed system property to do is to bootstrap a configset into ZooKeeper -- that's it. We don't need it to have special side meanings when a collection is created, which is the context we're looking at here. We'll need to communicate something in upgrades. I think we can say: > The system property "bootstrap_conf" (recently renamed) used to allow a collection creation command to default to the examination of system properties in the absence of proper creation parameters by the same name (like "collection.configName"). That no longer happens in Solr 10. Good riddance :-) -- 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]
[PR] SOLR-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
epugh opened a new pull request, #3604: URL: https://github.com/apache/solr/pull/3604 https://issues.apache.org/jira/browse/SOLR-17875 This is a more limited version of https://github.com/apache/solr/pull/3512. We are aiming to remove the dead bootstrapping of a core into a solr cloud, add a test for bootstrapping, and rename the variables. The boolean `bootstrap_conf` will hopefully be removed to as dead code. We are accepting fully qualified paths instead of relative in the bats test, and hoping not to trigger the Java Security Manager issues with the benchmarking tests. -- 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
epugh commented on PR #3604: URL: https://github.com/apache/solr/pull/3604#issuecomment-3246395965 All green here! Would love a LGTM so I can get this in and be done finally. -- 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
epugh commented on code in PR #3604: URL: https://github.com/apache/solr/pull/3604#discussion_r2317359598 ## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java: ## @@ -702,13 +700,6 @@ public static void createCollectionZkNode( } } -// if the config name wasn't passed in, use the default -if (!collectionProps.containsKey(ZkController.CONFIGNAME_PROP)) - collectionProps.put(ZkController.CONFIGNAME_PROP, defaultConfigName); Review Comment: Let me know what you think on this.. no tests failed... thoguh I am starting to realize we may have a lot of code paths with no tests in this area ;-( -- 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-17875: Redo of Rationalize bootstrap_conf, bootstrap_confdir, and -Dcollection.configName in start up scripts [solr]
epugh commented on code in PR #3604: URL: https://github.com/apache/solr/pull/3604#discussion_r2317358098 ## solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java: ## @@ -702,13 +700,6 @@ public static void createCollectionZkNode( } } -// if the config name wasn't passed in, use the default -if (!collectionProps.containsKey(ZkController.CONFIGNAME_PROP)) - collectionProps.put(ZkController.CONFIGNAME_PROP, defaultConfigName); Review Comment: I *think* that what I did was move this up to line 692, and removed the `if` conditional. This code path is only entered if you have a `solr.configset.bootstrap.confdir` set, so then we get the default either from the `solr.configset.bootstrap.config.name` or if that is missing then the `collection` variable... -- 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]
