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.

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

SOLR-15100: make ConfigSetService configurable in solr.xml #2343

Merged
merged 6 commits into from Mar 2, 2021
Merged

SOLR-15100: make ConfigSetService configurable in solr.xml #2343

merged 6 commits into from Mar 2, 2021

Conversation

baisui1981
Copy link
Contributor

Description

modify the static method ConfigSetService.createConfigSetService in that I can define the customized class name of ConfigSetService in solr.xml and create it in the method, https://issues.apache.org/jira/browse/SOLR-15100

Solution

modify the static method ConfigSetService.createConfigSetService , get property 'configSetService' which is name of customized ConfigSetService class name config in solr.xml and initialize it by java refect

Tests

add a test case in TestCoreContainer, test method is testCustomConfigSetService()

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the master branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Ref Guide (for Solr changes only).

@cffyh
Copy link

cffyh commented Feb 11, 2021

good job! I need this feature customized ConfigSetService

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Looks nice! I just have some minor feedback. Thank you for contributing!

@baisui1981
Copy link
Contributor Author

Looks nice! I just have some minor feedback. Thank you for contributing!

all the fix have done

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Can you please add a few lines to solr/solr-ref-guide/src/format-of-solr-xml.adoc about this?

Can you please also add a CHANGES.txt entry for 8.9 under Improvements?:

* SOLR-15100: Make the ConfigSetService pluggable/configurable via <string name="xxxxxx" /> in solr.xml
(your name here)

@@ -65,8 +65,8 @@ public static ConfigSetService createConfigSetService(NodeConfig nodeConfig, Sol
try {
Class<? extends ConfigSetService> clazz = loader.findClass(configSetServiceClass, ConfigSetService.class);
Constructor<? extends ConfigSetService> constructor
= clazz.getConstructor(SolrResourceLoader.class, Boolean.TYPE, ZkController.class);
return constructor.newInstance(loader, nodeConfig.hasSchemaCache(), zkController);
= clazz.getConstructor(SolrResourceLoader.class, NodeConfig.class, ZkController.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we can remove SolrResourceLoader as well, which is on NodeConfig.

@dsmiley
Copy link
Contributor

dsmiley commented Feb 28, 2021

Come to think of it, if the constructor was simply CoreContainer, it could get everything it might need? That would be very clean.

@baisui1981
Copy link
Contributor Author

Come to think of it, if the constructor was simply CoreContainer, it could get everything it might need? That would be very clean.

that's a good idea , Since that is so,shall i modify the method ConfigSetService.createConfigSetService() and simply the params list to one 'CoreContainer', that will be clean

@dsmiley
Copy link
Contributor

dsmiley commented Feb 28, 2021

shall i modify the method ConfigSetService.createConfigSetService() and simply the params list to one 'CoreContainer', that will be clean

Great idea!

@dsmiley dsmiley merged commit 8b44342 into apache:master Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants