[GitHub] [solr] dsmiley commented on a diff in pull request #943: SOLR-15342: Separate out a SolrJ-Zookeeper module

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@solr.apache.org

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