You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:14:22 UTC
[GitHub] [solr] dsmiley commented on a change in pull request #17: SOLR-14341: Move configName into DocCollection class
dsmiley commented on a change in pull request #17:
URL: https://github.com/apache/solr/pull/17#discussion_r645742672
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -233,6 +233,41 @@ public static ClusterState createFromJson(int version, byte[] bytes, Set<String>
return createFromCollectionMap(version, stateMap, liveNodes);
}
+ /**
+ *
+ * Backward compatibility reason
+ * This can be removed in Solr 10
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonWithConfigName(int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
+ if (bytes == null || bytes.length == 0) {
+ return new ClusterState(liveNodes, Collections.<String, DocCollection>emptyMap());
Review comment:
You can skip the explicit types in the generics; juse use a diamond. IntelliJ should show this for you automatically to help you.
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -233,6 +233,41 @@ public static ClusterState createFromJson(int version, byte[] bytes, Set<String>
return createFromCollectionMap(version, stateMap, liveNodes);
}
+ /**
+ *
Review comment:
Always begin javadocs with a statement of what the thing being documented is/does; okay to be brief if you like. *Then* say whatever other things you want to say. End sentences with a period; the whole block is basically HTML and thus a newline is not displayed this way in javadocs. IntelliJ recently has the option of rendering the HTML, which is nice, and would make improperly formatted Javadocs look bad.
##########
File path: solr/core/src/java/org/apache/solr/cloud/DistributedClusterStateUpdater.java
##########
@@ -524,7 +524,11 @@ private ClusterState fetchStateForCollection() throws KeeperException, Interrupt
String collectionStatePath = ZkStateReader.getCollectionPath(updater.getCollectionName());
Stat stat = new Stat();
byte[] data = zkStateReader.getZkClient().getData(collectionStatePath, null, stat, true);
- ClusterState clusterState = ClusterState.createFromJson(stat.getVersion(), data, Collections.emptySet());
+
+ // If configName is not in state.json, we read it from old location due to backward compatibility reason
+ // TODO in Solr 10 this code should no longer read configName from old place
Review comment:
```suggestion
// This factory method can detect a missing configName and supply it by reading it from the old ZK location.
// TODO in Solr 10 remove that factory method
```
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
##########
@@ -1428,7 +1428,11 @@ private DocCollection fetchCollectionState(String coll, Watcher watcher) throws
try {
Stat stat = new Stat();
byte[] data = zkClient.getData(collectionPath, watcher, stat, true);
- ClusterState state = ClusterState.createFromJson(stat.getVersion(), data, Collections.emptySet());
+
+ // If configName is not in state.json, we read it from old location due to backward compatibility reason
Review comment:
You can copy my suggested comment edits from the other call-site.
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -233,6 +233,41 @@ public static ClusterState createFromJson(int version, byte[] bytes, Set<String>
return createFromCollectionMap(version, stateMap, liveNodes);
}
+ /**
+ *
+ * Backward compatibility reason
+ * This can be removed in Solr 10
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonWithConfigName(int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
Review comment:
Naming is hard here. createFromJsonSupportingLegacyConfigName ?
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -233,6 +233,41 @@ public static ClusterState createFromJson(int version, byte[] bytes, Set<String>
return createFromCollectionMap(version, stateMap, liveNodes);
}
+ /**
+ *
+ * Backward compatibility reason
+ * This can be removed in Solr 10
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonWithConfigName(int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
+ if (bytes == null || bytes.length == 0) {
+ return new ClusterState(liveNodes, Collections.<String, DocCollection>emptyMap());
+ }
+ Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(bytes);
+ if (stateMap.containsKey(coll)) {
+ Map<String, Object> props = (Map<String, Object>) stateMap.get(coll);
Review comment:
You could do this _before_ containsKey and thus not have to call containsKey; just check if props is null.
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -233,6 +233,41 @@ public static ClusterState createFromJson(int version, byte[] bytes, Set<String>
return createFromCollectionMap(version, stateMap, liveNodes);
}
+ /**
+ *
+ * Backward compatibility reason
+ * This can be removed in Solr 10
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonWithConfigName(int version, byte[] bytes, Set<String> liveNodes, String coll, SolrZkClient zkClient) {
+ if (bytes == null || bytes.length == 0) {
+ return new ClusterState(liveNodes, Collections.<String, DocCollection>emptyMap());
+ }
+ Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(bytes);
+ if (stateMap.containsKey(coll)) {
+ Map<String, Object> props = (Map<String, Object>) stateMap.get(coll);
+ if (!props.containsKey(ZkStateReader.CONFIGNAME_PROP)) {
+ try {
+ // read configName from collections/collection node
+ String path = ZkStateReader.COLLECTIONS_ZKNODE + "/" + coll;
+ byte[] data = zkClient.getData(path, null, null, true);
+ if (data != null && data.length > 0) {
+ ZkNodeProps configProp = ZkNodeProps.load(data);
+ String configName = configProp.getStr(ZkStateReader.CONFIGNAME_PROP);
+ if (configName != null) {
+ props.put(ZkStateReader.CONFIGNAME_PROP, configName);
+ stateMap.put(coll, props);
Review comment:
If we ultimately can't find the config name in the old location (for whatever reason), wouldn't this deserve logging a warning?
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java
##########
@@ -233,6 +233,41 @@ public static ClusterState createFromJson(int version, byte[] bytes, Set<String>
return createFromCollectionMap(version, stateMap, liveNodes);
}
+ /**
+ *
Review comment:
A comment on createFromJson linking to this method could say that the other method doesn't support legacy configName location and thus don't call it where that's important.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -560,10 +559,16 @@ public static void createCollectionZkNode(DistribStateManager stateManager, Stri
}
collectionProps.remove(ZkStateReader.NUM_SHARDS_PROP); // we don't put numShards in the collections properties
+ collectionProps.remove(ZkStateReader.CONFIGNAME_PROP); // we don't write configName on a zk collection node
- ZkNodeProps zkProps = new ZkNodeProps(collectionProps);
- stateManager.makePath(collectionPath, Utils.toJSON(zkProps), CreateMode.PERSISTENT, false);
-
+ if (collectionProps.size() > 0) {
+ ZkNodeProps zkProps = new ZkNodeProps(collectionProps);
+ // create a node with collectionProps data
+ stateManager.makePath(collectionPath, Utils.toJSON(zkProps), CreateMode.PERSISTENT, false);
Review comment:
In some separate issue, if I get around to it, I'll propose dropping these props here since I think nothing reads it.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -370,7 +369,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).equals(DEFAULT_CONFIGSET_NAME) || ConfigSetsHandler.isAutoGeneratedConfigSet(message.getStr(COLL_CONF));
Review comment:
I don't recall why you modified this condition to include this.
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java
##########
@@ -55,6 +56,7 @@
private final int znodeVersion;
private final String name;
+ private String configName;
Review comment:
Why is it not final?
--
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@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org