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:
[email protected]
