dsmiley commented on code in PR #943:
URL: https://github.com/apache/solr/pull/943#discussion_r930468981
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/util/ZkUtils.java:
##########
@@ -32,4 +40,45 @@ public static Map<String, Object> getJson(
}
return Collections.emptyMap();
}
+
+ @SuppressWarnings({"unchecked"})
+ public static Map<String, Object> getJson(DistribStateManager
distribStateManager, String path)
Review Comment:
You can move this method to be a method directly on DistribStateManager,
thus not needing to take distribStateManager as an argument, as it'd be "this".
##########
solr/test-framework/src/java/org/apache/solr/cloud/CloudSolrClientUtils.java:
##########
@@ -0,0 +1,109 @@
+package org.apache.solr.cloud;
+
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.impl.ZkClientClusterStateProvider;
+import org.apache.solr.common.cloud.CollectionStatePredicate;
+import org.apache.solr.common.cloud.CollectionStateWatcher;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.DocCollectionWatcher;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Predicate;
+
+import org.apache.solr.common.cloud.ZkStateReader;
+
+public class CloudSolrClientUtils {
Review Comment:
These methods are all not-needed I think, thus we don't need this utility
class holder either. First, observe that "connect" effectively does nothing
for the ZK implementation. Thus these methods (waitForState, ...) here that
call it and then immediately call assertZKStateProvider mean that calling
connect() first is pointless. These methods are effectively one-liners at that
point and you can inline them. IntelliJ makes this trial work via the "inline"
refactoring.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/StatementImpl.java:
##########
@@ -76,9 +75,8 @@ private ResultSet executeQueryImpl(String sql) throws
SQLException {
protected SolrStream constructStream(String sql) throws IOException {
try {
- ZkStateReader zkStateReader =
ZkStateReader.from(this.connection.getClient());
Slice[] slices =
- CloudSolrStream.getSlices(this.connection.getCollection(),
zkStateReader, true);
+ CloudSolrStream.getSlices(this.connection.getCollection(),
this.connection.getClient(), true);
Review Comment:
This particular commit is nice; it will allow a SolrJ client using streaming
expressions to not require ZK needlessly. At least it's a step in that
direction if not totally there. CC @joel-bernstein
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -700,25 +675,6 @@ public static SpecProvider getSpec(final String name) {
};
}
- public static String parseMetricsReplicaName(String collectionName, String
coreName) {
Review Comment:
Looks like you moved this and realized you didn't need to. But when it came
back; it's in a different spot in this class.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -128,7 +130,7 @@ public DocCollection(
}
this.activeSlicesArr = activeSlices.values().toArray(new
Slice[activeSlices.size()]);
this.router = router;
- this.znode = ZkStateReader.getCollectionPath(name);
+ this.znode = "/collections/" + name + "/state.json"; // nocommit : check
if this needs to become generic
Review Comment:
LGTM
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/util/ZkUtils.java:
##########
@@ -0,0 +1,35 @@
+package org.apache.solr.common.util;
+
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkOperation;
+import org.apache.zookeeper.KeeperException;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class ZkUtils {
Review Comment:
Don't create another utility class. In this case, the method is used in
exactly one spot and so you can inline it.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -67,7 +67,51 @@ public ZkClientClusterStateProvider(String zkHost) {
this.zkHost = zkHost;
}
- @Override
+ /**
+ * Create a ClusterState from Json. This method supports legacy configName
location
+ *
+ * @param bytes a byte array of a Json representation of a mapping from
collection name to the
+ * Json representation of a {@link DocCollection} as written by {@link
ClusterState#write(JSONWriter)}. It
+ * can represent one or more collections.
+ * @param liveNodes list of live nodes
+ * @param coll collection name
+ * @param zkClient ZK client
+ * @return the ClusterState
+ */
+ @SuppressWarnings({"unchecked"})
+ @Deprecated
+ public static ClusterState createFromJsonSupportingLegacyConfigName(
Review Comment:
@HoustonPutman I observed that in #749 you put this on ZkStateReader. Is
ZkClientclusterStateProvider fine? I don't have a strong opinion.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkCmdExecutor.java:
##########
@@ -44,7 +43,7 @@ public ZkCmdExecutor(int timeoutms) {
* class.
*/
public ZkCmdExecutor(int timeoutms, IsClosed isClosed) {
- timeouts = timeoutms / 1000.0;
+ double timeouts = timeoutms / 1000.0;
Review Comment:
not a big deal but please refrain from making little changes like this
(probably suggested by your IDE) unless you need to touch a piece of code any
way. (moving a class doesn't count)
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/sql/DatabaseMetaDataImpl.java:
##########
@@ -118,7 +118,7 @@ public String getDatabaseProductVersion() throws
SQLException {
SolrClient solrClient = null;
for (String node : liveNodes) {
try {
- String nodeURL =
ZkStateReader.from(cloudSolrClient).getBaseUrlForNodeName(node);
+ String nodeURL = Utils.getBaseUrlForNodeName(node,
/*getClusterProperty(URL_SCHEME, "http")*/ "http");
Review Comment:
That's "nocommit" worthy :-)
I suppose CloudSolrClient.getClusterStateProvider should have a
getBaseUrlForNodeName() instance method. The ZK implementation could call the
one on ZkStateReader so that this scheme is consulted.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -293,7 +293,7 @@ public static Slice[] getSlices(
// check for alias or collection
- List<String> allCollections = new ArrayList<>();
+ List<String> allCollections = new ArrayLigist<>();
Review Comment:
Please compile and tidy before committing & pushing :-)
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/util/ZkUtils.java:
##########
@@ -32,4 +40,45 @@ public static Map<String, Object> getJson(
}
return Collections.emptyMap();
}
+
+ @SuppressWarnings({"unchecked"})
+ public static Map<String, Object> getJson(DistribStateManager
distribStateManager, String path)
+ throws InterruptedException, IOException, KeeperException {
+ VersionedData data;
+ try {
+ data = distribStateManager.getData(path);
+ } catch (KeeperException.NoNodeException | NoSuchElementException e) {
+ return Collections.emptyMap();
+ }
+ if (data == null || data.getData() == null || data.getData().length == 0)
+ return Collections.emptyMap();
+ return (Map<String, Object>) Utils.fromJSON(data.getData());
+ }
+
+ public static InputStream toJavabin(Object o) throws IOException {
Review Comment:
Move to a utility method on JavaBinCodec.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/DeepRandomStream.java:
##########
@@ -282,50 +277,6 @@ public List<TupleStream> children() {
return solrStreams;
}
- public static Slice[] getSlices(
Review Comment:
Where did this method go?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/CloudSolrStream.java:
##########
@@ -347,14 +346,14 @@ private StreamComparator parseComp(String sort, String
fl) throws IOException {
}
public static Slice[] getSlices(
Review Comment:
@joel-bernstein We are changing the API signature. Is this fine to bring
to Solr 8 -- e.g. is it obscure? If it's a problem, this change would not
occur and instead the CloudSolrStream stuff would need to go to the
SolrJ-Zookeeper module. I suppose we should just do that. Not an action to
take now but something to remember in the backport.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/Slice.java:
##########
@@ -43,6 +43,9 @@
public class Slice extends ZkNodeProps implements Iterable<Replica> {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // nocommit : to move this from ZkStateReader
+ private static final String STATE_PROP = "state";
Review Comment:
This is a good new home.
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -47,6 +41,14 @@
public class DocCollection extends ZkNodeProps implements Iterable<Slice> {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ // nocommit : to move these from ZkStateReader
Review Comment:
I think this is a good home for these properties now.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
