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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
