[GitHub] [solr] justinrsweeney commented on a diff in pull request #1258: Refactoring to move live nodes handling out of ZkStateReader and into own class

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

justinrsweeney commented on code in PR #1258:
URL: https://github.com/apache/solr/pull/1258#discussion_r1060209357


##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -679,7 +679,13 @@ private void constructState(Set<String> changedCollections) {
       result.putIfAbsent(entry.getKey(), entry.getValue());
     }
 
-    this.clusterState = new ClusterState(result, liveNodes);
+    this.clusterState = new ClusterState(result, zkLiveNodes.getLiveNodes());
+    // Listen for future live nodes changes to update ClusterState
+    zkLiveNodes.addLiveNodesListener(
+        (o, n) -> {
+          clusterState.setLiveNodes(n);
+          return false;
+        });

Review Comment:
   This is new, but none of this is really tested yet.
   
   The idea I'm thinking here is to create the ClusterState once and then have classes for each data type in ZK handling updates and updating necessary places via listeners like here: https://github.com/apache/solr/pull/1258/files/144d380843439672b2b7e76dae6fc6c94f839924#diff-4710a20b3d893bec0637610d3cce7dc383ca328c68a6fb0d4c7266e2041b9d0bR90, instead of having a bunch of methods in ZkStateReader that update ClusterState for all sort of data types.



-- 
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