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

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

dsmiley commented on a change in pull request #17:
URL: https://github.com/apache/solr/pull/17#discussion_r594760829



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CollApiCmds.java
##########
@@ -226,8 +226,8 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
 
       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);
+      //configName will be put in collectionProps so that it will appear in state.json
+      String configName = (String) message.getProperties().get(CollectionAdminParams.COLL_CONF);
 
       if (configName != null) {

Review comment:
       In this block, the old location that holds confName is updated and then the collection is reloaded. I think we should guarantee that reloading the collection will "see" the new state (with configName) but that must occur *after* the state is updated.  Thus this logic should probably move further down in this method.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -136,6 +136,8 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
       // this also creates the collection zk node as a side-effect
       CollectionHandlingUtils.createConfNode(stateManager, configName, collectionName);

Review comment:
       Note that createConfNode will somehow move to ConfigSetService in the other PR

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java
##########
@@ -169,20 +169,15 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
       }
 
       // delete related config set iff: it is auto generated AND not related to any other collection
-      String configSetName = zkStateReader.readConfigName(collection);
+      String configSetName = coll.getConfigName(zkStateReader);
 
       if (ConfigSetsHandler.isAutoGeneratedConfigSet(configSetName)) {

Review comment:
       That utility method here may be useful in the other spot you were using String.contains

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
##########
@@ -238,7 +238,6 @@ public boolean canBeRemoved() {
 
   /**
    * Returns config set name for collection.
-   * TODO move to DocCollection (state.json).
    *

Review comment:
       Mark deprecated as 9.0; point to the new location

##########
File path: solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java
##########
@@ -90,8 +91,8 @@ public static StorageIO newStorageIO(String collection, SolrResourceLoader resou
     if (resourceLoader instanceof ZkSolrResourceLoader) {
       zkClient = ((ZkSolrResourceLoader)resourceLoader).getZkController().getZkClient();
       try {
-        zkConfigName = ((ZkSolrResourceLoader)resourceLoader).getZkController().
-            getZkStateReader().readConfigName(collection);
+        final ZkStateReader zkStateReader = ((ZkSolrResourceLoader)resourceLoader).getZkController().getZkStateReader();

Review comment:
       FYI in the other PR to consolidate ConfigService, it looks like it'll need to consider this class (ManagedResourceStorage) which is making assumptions as to where the config data is.

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -206,6 +211,28 @@ public String getName() {
     return name;
   }
 
+  /**
+   * Return config name or null if solr version is 8x or below
+   */
+  public String getConfigName() { return configName; }
+
+  /**
+   * Return non-null config name
+   */
+  public String getConfigName(ZkStateReader zkStateReader) {
+    final String configSetName;

Review comment:
       Needn't define this var; just return directly either configName or readConfigName()

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -374,7 +376,7 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
         log.debug("Finished create command on all shards for collection: {}", collectionName);
         // Emit a warning about production use of data driven functionality
         boolean defaultConfigSetUsed = message.getStr(COLL_CONF) == null ||
-            message.getStr(COLL_CONF).equals(DEFAULT_CONFIGSET_NAME);
+            message.getStr(COLL_CONF).contains(DEFAULT_CONFIGSET_NAME);

Review comment:
       use of contains is often sloppy (tests are fine).  Someone might name their collection my data_default.
   Am I right that this change here is a minor improvement to log a warning that otherwise wouldn't of happened?  (it's fine but just checking)




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