dsmiley commented on a change in pull request #17:
URL: https://github.com/apache/solr/pull/17#discussion_r641833458
##########
File path:
solr/core/src/test/org/apache/solr/cloud/api/collections/SimpleCollectionCreateDeleteTest.java
##########
@@ -110,7 +110,7 @@ public void testDeleteAlsoDeletesAutocreatedConfigSet()
throws Exception {
// collection exists now
assertTrue(cloudClient.getZkStateReader().getZkClient().exists(ZkStateReader.COLLECTIONS_ZKNODE
+ "/" + collectionName, false));
- String configName =
cloudClient.getZkStateReader().readConfigName(collectionName);
+ String configName =
cloudClient.getZkStateReader().getClusterState().getCollection(collectionName).getConfigName();
Review comment:
Instead:
```
String configName =
cloudClient.getClusterStateProvider().getCollection(collectionName).getConfigName();
```
Please make the equivalent change elsewhere across this PR.
##########
File path:
solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java
##########
@@ -109,11 +110,27 @@ public ZkWriteCommand createCollection(ClusterState
clusterState, ZkNodeProps me
collectionProps.put("autoCreated", "true");
}
+ addConfigNameToProps(message, collectionProps);
+
DocCollection newCollection = new DocCollection(cName, slices,
collectionProps, router, -1);
return new ZkWriteCommand(cName, newCollection);
}
+ public static void addConfigNameToProps(ZkNodeProps message, Map<String,
Object> collectionProps) {
Review comment:
I see you factored this out because there is another caller of this
logic for modifying a collection that you added. It makes me wonder why more
code couldn't be factored out as there are a varitety of metadata on a
collection? Hmm; maybe see. Perhaps public->package scope as well?
##########
File path:
solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateReaderTest.java
##########
@@ -97,14 +99,17 @@ public void testCollectionStateWatcherCaching() throws
Exception {
zkClient.makePath(ZkStateReader.COLLECTIONS_ZKNODE + "/c1", true);
ZkStateWriter writer = new ZkStateWriter(reader, new Stats());
- DocCollection state = new DocCollection("c1", new HashMap<>(), new
HashMap<>(), DocRouter.DEFAULT, 0);
+ DocCollection state = new DocCollection("c1", new HashMap<>(),
Collections.singletonMap(ZkStateReader.CONFIGNAME_PROP,
ConfigSetsHandler.DEFAULT_CONFIGSET_NAME), DocRouter.DEFAULT, 0);
ZkWriteCommand wc = new ZkWriteCommand("c1", state);
writer.enqueueUpdate(reader.getClusterState(),
Collections.singletonList(wc), null);
writer.writePendingUpdates();
assertTrue(zkClient.exists(ZkStateReader.COLLECTIONS_ZKNODE +
"/c1/state.json", true));
reader.waitForState("c1", 1, TimeUnit.SECONDS, (liveNodes,
collectionState) -> collectionState != null);
- state = new DocCollection("c1", new HashMap<>(),
Collections.singletonMap("x", "y"), DocRouter.DEFAULT, 0);
+ Map<String, Object> props = new HashMap<>();
Review comment:
Map.of would be so much nicer here
##########
File path:
solr/core/src/test/org/apache/solr/cloud/OverseerCollectionConfigSetProcessorTest.java
##########
@@ -563,6 +564,9 @@ private void handleCreateCollMessageProps(ZkNodeProps
props) {
try {
if
(CollectionParams.CollectionAction.CREATE.isEqual(props.getStr("operation"))) {
String collName = props.getStr("name");
+ if (props.containsKey(CollectionAdminParams.COLL_CONF)) {
+ props.plus(ZkStateReader.CONFIGNAME_PROP,
props.getProperties().remove("collection.configName"));
Review comment:
"plus" returns a new ZkNodeProps without modifying it in-place! Maybe
you don't need this code any way? (I dunno)
##########
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:
I filed an issue: https://issues.apache.org/jira/browse/SOLR-15445 (not
critial)
##########
File path:
solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java
##########
@@ -109,11 +110,27 @@ public ZkWriteCommand createCollection(ClusterState
clusterState, ZkNodeProps me
collectionProps.put("autoCreated", "true");
}
+ addConfigNameToProps(message, collectionProps);
+
DocCollection newCollection = new DocCollection(cName, slices,
collectionProps, router, -1);
return new ZkWriteCommand(cName, newCollection);
}
+ public static void addConfigNameToProps(ZkNodeProps message, Map<String,
Object> collectionProps) {
+ // put configName in props so that it will appear in state.json
+ final String configName = (String)
message.getProperties().get(CollectionAdminParams.COLL_CONF);
+
+ if (configName != null) {
+ collectionProps.put(ZkStateReader.CONFIGNAME_PROP, configName);
+ }
+
+ // if collection.configName is already in props rename it to configName
+ if (collectionProps.containsKey(CollectionAdminParams.COLL_CONF)) {
Review comment:
"collection.configName" should not be in props (aka not in state.json).
We could add an assert if you think it gets here to troubleshoot why so we can
chase down the root cause.
##########
File path:
solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateReaderTest.java
##########
@@ -60,7 +62,7 @@ public void testExternalCollectionWatchedNotWatched() throws
Exception{
// create new collection
ZkWriteCommand c1 = new ZkWriteCommand("c1",
- new DocCollection("c1", new HashMap<>(), new HashMap<>(),
DocRouter.DEFAULT, 0));
+ new DocCollection("c1", new HashMap<>(),
Collections.singletonMap(ZkStateReader.CONFIGNAME_PROP,
ConfigSetsHandler.DEFAULT_CONFIGSET_NAME), DocRouter.DEFAULT, 0));
Review comment:
As we get used to Java 11, I think we'll prefer `Map.of` which is
shorter. This is still fine though.
##########
File path:
solr/core/src/java/org/apache/solr/cloud/api/collections/CollApiCmds.java
##########
@@ -322,6 +319,13 @@ public void call(ClusterState clusterState, ZkNodeProps
message, @SuppressWarnin
for (Map.Entry<String, Object> updateEntry :
message.getProperties().entrySet()) {
String updateKey = updateEntry.getKey();
+ /** update key from collection.configName to configName; actual
renaming happens in
Review comment:
Very minor but you wrote this in Javadoc style yet javadocs cannot be
located on inline code, they can only be on class elements. So just edit it to
be a couple lines with "//".
I very much like the comment though -- great to see hints like this for
readers! This is a complicated puzzle.
--
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...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
