[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2391: SOLR-14341: Move configName into DocCollection class

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #2391:
URL: https://github.com/apache/lucene-solr/pull/2391#discussion_r590352385



##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -300,6 +307,9 @@ public Integer getReplicationFactor() {
     return replicationFactor;
   }
 
+  /**
+   * Return non-null configName.

Review comment:
       ```suggestion
      * Return non-null configset name.
   ```

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -88,10 +88,17 @@ public DocCollection(String name, Map<String, Slice> slices, Map<String, Object>
    * @param zkVersion The version of the Collection node in Zookeeper (used for conditional updates).
    */
   public DocCollection(String name, Map<String, Slice> slices, Map<String, Object> props, DocRouter router, int zkVersion) {
-    super(props);
-    if (props == null || props.containsKey("baseConfigSet")) {
+    if (props == null) {

Review comment:
       null || props.isEmpty() can be combined

##########
File path: solr/core/src/java/org/apache/solr/cloud/CloudConfigSetService.java
##########
@@ -86,12 +86,11 @@ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
 
     // The configSet is read from ZK and populated.  Ignore CD's pre-existing configSet; only populated in standalone
     final String configSetName;
-    try {
-      configSetName = zkController.getZkStateReader().readConfigName(colName);
-      cd.setConfigSet(configSetName);
-    } catch (KeeperException ex) {
-      throw new ZooKeeperException(SolrException.ErrorCode.SERVER_ERROR, "Trouble resolving configSet for collection " + colName + ": " + ex.getMessage());
+    configSetName = zkController.getZkStateReader().getClusterState().getCollection(colName).getConfigName();
+    if (configSetName == null) {

Review comment:
       still an issue?

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java
##########
@@ -554,12 +554,11 @@ private void modifyCollection(ClusterState clusterState, ZkNodeProps message, @S
     final String collectionName = message.getStr(ZkStateReader.COLLECTION_PROP);
     //the rest of the processing is based on writing cluster state properties
     //remove the property here to avoid any errors down the pipeline due to this property appearing
-    String configName = (String) message.getProperties().remove(CollectionAdminParams.COLL_CONF);
+    String configName = (String) message.getProperties().get(CollectionAdminParams.COLL_CONF);

Review comment:
       just observing that this innocent looking change seems important to this PR.  Previously this data had disappeared from the state.

##########
File path: solr/core/src/java/org/apache/solr/cloud/OverseerConfigSetMessageHandler.java
##########
@@ -367,12 +367,7 @@ private void deleteConfigSet(String configSetName, boolean force) throws IOExcep
 
     for (Map.Entry<String, DocCollection> entry : zkStateReader.getClusterState().getCollectionsMap().entrySet()) {
       String configName = null;
-      try {
-        configName = zkStateReader.readConfigName(entry.getKey());
-      } catch (KeeperException ex) {
-        throw new SolrException(ErrorCode.BAD_REQUEST,
-            "Can not delete ConfigSet as it is currently being used by collection [" + entry.getKey() + "]");
-      }
+      configName = entry.getValue().getConfigName();

Review comment:
       combine declaration and initialization




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org