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



##########
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:
       I think we can have getConfigName never return null?  That would be ideal, any way.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java
##########
@@ -166,7 +166,7 @@ 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 = message.getStr(ZkStateReader.COLLECTION_CONFIG_PROP, coll.getConfigName());

Review comment:
       Like my last comment; I'm unsure `message` has the configSet

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -81,7 +82,7 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
     String backupName = message.getStr(NAME);
     String repo = message.getStr(CoreAdminParams.BACKUP_REPOSITORY);
     boolean incremental = message.getBool(CoreAdminParams.BACKUP_INCREMENTAL, true);
-    String configName = ocmh.zkStateReader.readConfigName(collectionName);
+    String configName = message.getStr(ZkStateReader.COLLECTION_CONFIG_PROP);

Review comment:
       Are you sure that `message` contains the configSet?  (Maybe it does; I don't know).  But what I do know is that you can look it up from DocCollection now.

##########
File path: solr/core/src/java/org/apache/solr/cloud/overseer/CollectionMutator.java
##########
@@ -99,7 +100,7 @@ public ZkWriteCommand deleteShard(final ClusterState clusterState, ZkNodeProps m
   }
 
   public ZkWriteCommand modifyCollection(final ClusterState clusterState, ZkNodeProps message) {
-    if (!checkCollectionKeyExistence(message)) return ZkStateWriter.NO_OP;
+    if (!checkCollectionKeyExistence(message) || !checkKeyExistence(message, ZkStateReader.COLLECTION_CONFIG_PROP)) return ZkStateWriter.NO_OP;

Review comment:
       This change is curious to me.    Does this mean the configSet (aka configName in much of the naming) is mandatory for modifying a collection?  If so, that seems wrong.

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -84,7 +88,10 @@ 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==null ? props = new HashMap<>() : props);
+    super(props);
+    if (props == null || props.containsKey("baseConfigSet")) {

Review comment:
       Why the name "baseConfigSet"?  And if it is in props, wouldn't we end up over-writing CONFIGNAME_PROP if it's there?
   
   If props is null, how do we call props.put without getting an NPE?

##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ClusterStatus.java
##########
@@ -139,14 +139,9 @@ public void getClusterStatus(@SuppressWarnings({"rawtypes"})NamedList results)
       if (collectionVsAliases.containsKey(name) && !collectionVsAliases.get(name).isEmpty()) {
         collectionStatus.put("aliases", collectionVsAliases.get(name));
       }
-      try {
-        String configName = zkStateReader.readConfigName(name);
-        collectionStatus.put("configName", configName);
-        collectionProps.add(name, collectionStatus);
-      } catch (KeeperException.NoNodeException ex) {
-        // skip this collection because the configset's znode has been deleted
-        // which can happen during aggressive collection removal, see SOLR-10720
-      }
+      String configName = message.getStr(ZkStateReader.COLLECTION_CONFIG_PROP, clusterStateCollection.getConfigName());

Review comment:
       The message didn't have this before (I assume?); does it now?

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -119,6 +126,10 @@ public DocCollection(String name, Map<String, Slice> slices, Map<String, Object>
     this.activeSlicesArr = activeSlices.values().toArray(new Slice[activeSlices.size()]);
     this.router = router;
     this.znode = ZkStateReader.getCollectionPath(name);
+    this.configName = String.valueOf(props.get(CONFIGNAME_PROP)) == null ? String.valueOf(props.get(COLLECTION_CONFIG_PROP)) : String.valueOf(props.get(CONFIGNAME_PROP));

Review comment:
       Pick one -- probably CONFIGNAME_PROP, not also COLLECTION_CONFIG_PROP.  I believe COLLECTION_CONFIG_PROP is only used at the HTTP API level, not internally.

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -289,6 +300,8 @@ public Integer getReplicationFactor() {
     return replicationFactor;
   }
 
+  public String getConfigName() { return configName; }

Review comment:
       Add a javadoc to say does not return null.

##########
File path: solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java
##########
@@ -91,7 +91,7 @@ public static StorageIO newStorageIO(String collection, SolrResourceLoader resou
       zkClient = ((ZkSolrResourceLoader)resourceLoader).getZkController().getZkClient();
       try {
         zkConfigName = ((ZkSolrResourceLoader)resourceLoader).getZkController().
-            getZkStateReader().readConfigName(collection);
+            getZkStateReader().getClusterState().getCollection(collection).getConfigName();

Review comment:
       BTW for brevity, you can remove `getZkStateReader().` here and elsewhere since ZkController has a convenience method for the cluster state.

##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
##########
@@ -128,6 +128,7 @@
 
   public static final String CONFIGS_ZKNODE = "/configs";
   public final static String CONFIGNAME_PROP = "configName";
+  public final static String COLLECTION_CONFIG_PROP = "collection.configName";

Review comment:
       Probably doesn't go here because I think it's specific to the HTTP API layer.  This class is too internal to declare such a name.




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