patsonluk commented on code in PR #2438:
URL: https://github.com/apache/solr/pull/2438#discussion_r1628525202
##########
solr/core/src/java/org/apache/solr/core/ConfigSetService.java:
##########
@@ -391,6 +402,21 @@ protected NamedList<Object>
loadConfigSetFlags(SolrResourceLoader loader) throws
*/
protected abstract SolrResourceLoader
createCoreResourceLoader(CoreDescriptor cd);
+ /**
+ * Create a SolrResourceLoader for a core with the provided configSetName.
+ *
+ * <p>By default, this will just call {@link
+ * ConfigSetService#createConfigSetService(CoreContainer)}. Child
implementation might override
+ * this to make use of the configSetName directly
+ *
+ * @param cd the core's CoreDescriptor
+ * @param configSetName an optional config set name
+ * @return a SolrResourceLoader
+ */
+ protected SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd,
String configSetName) {
Review Comment:
So i tried removing
```
String configSetName =
zkController.getClusterState().getCollection(cd.getCollectionName()).getConfigName();
cd.setConfigSet(configSetName);
```
Then the core loading starts failing. With some investigation:
1. `CoreDescriptor` instantiation does not set the `configSet` in the
`coreProperties` via both admin create path and startup core discovery path.
Those paths would set `collection.configName` instead.
2. Therefore without setting it as a part of
`ZkConfigSetService#createCoreResourceLoader`, `cd.getConfigSet` would give
null, and could trigger error when calling `new ZkSolrResourceLoader(
cd.getInstanceDir(), cd.getConfigSet(),
parentLoader.getClassLoader(), zkController)` (or anywhere else relies on
`CoreDescriptor#getgetConfigSet`
I'm not 100% sure if calling `cd.setConfigSet(configSetName)` is a good
thing to do within `createCoreResourceLoader` since it appears to be an
unexpected side effect. However, to focus on synthetic core alone. I pushed a
commit to do a null check with a side note
```
// Currently, cd.getConfigSet() is always null. Except that it's
explicitly set by
// {@link org.apache.solr.core.SyntheticSolrCore}.
// Should we consider setting it for all cores as a part of
CoreDescriptor creation/loading
// process?
if (cd.getConfigSet() == null) {
String configSetName =
zkController.getClusterState().getCollection(cd.getCollectionName()).getConfigName();
cd.setConfigSet(configSetName);
}
```
Any thoughts? @dsmiley 😊
--
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
