dsmiley commented on code in PR #2585:
URL: https://github.com/apache/solr/pull/2585#discussion_r1690279665
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -868,7 +869,11 @@ public ClusterState getClusterState() {
}
public Object getUpdateLock() {
- return this;
+ return updateLock;
Review Comment:
Scope is becoming slightly more than a refactor; now getUpdateLock has a
dedicated object. Was that necessary to achieve the refactoring aims? For
example it creates new problems... note that
`createClusterStateWatchersAndUpdate()` is marked synchronized, thus it
synchronizes on "this" but now it's different than what getUpdateLock returns.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -26,11 +25,17 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Fetches and manages collection properties from a ZooKeeper ensemble using
{@link ZkStateReader}
+ * for ZooKeeper client access. Ensures thread safety by synchronizing on the
update lock provided
+ * by {@link ZkStateReader}
Review Comment:
IMO this is a detail (that may change) that isn't warranted in class
javadocs.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -0,0 +1,406 @@
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptyMap;
+
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.solr.common.SolrCloseable;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader.CollectionWatch;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Fetches and manages collection properties from a ZooKeeper ensemble using
{@link ZkStateReader}
+ * for ZooKeeper client access. Ensures thread safety by synchronizing on the
update lock provided
+ * by {@link ZkStateReader}
+ */
+public class CollectionPropertiesZkStateReader implements SolrCloseable {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private volatile boolean closed = false;
Review Comment:
It's not clear we need a new "closed" boolean independent from
ZkStateReader. Since you've added ZkStateReader as a field, it's easy to call
ZkStateReader.isClosed. I understand my suggestion tightly couples this class
to ZkStateReader but... what's wrong with that? That's the reality here; heck
look at its name.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -0,0 +1,406 @@
+package org.apache.solr.common.cloud;
+
+import static java.util.Collections.emptyMap;
+
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.solr.common.SolrCloseable;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader.CollectionWatch;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.WatchedEvent;
+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Fetches and manages collection properties from a ZooKeeper ensemble using
{@link ZkStateReader}
+ * for ZooKeeper client access. Ensures thread safety by synchronizing on the
update lock provided
+ * by {@link ZkStateReader}
+ */
+public class CollectionPropertiesZkStateReader implements SolrCloseable {
Review Comment:
Implementing Closeable (not SolrCloseable) is adequate since we don't need
the isClosed distinction (I think).
--
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-unsubscr...@solr.apache.org
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
