dsmiley commented on code in PR #2935:
URL: https://github.com/apache/solr/pull/2935#discussion_r1920949678


##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -226,17 +232,15 @@ public Set<String> getLiveNodes() {
     }
     if (TimeUnit.SECONDS.convert((System.nanoTime() - liveNodesTimestamp), 
TimeUnit.NANOSECONDS)
         > getCacheTimeout()) {
-      for (String nodeName : liveNodes) {
-        String baseUrl = Utils.getBaseUrlForNodeName(nodeName, urlScheme);
-        try (SolrClient client = getSolrClient(baseUrl)) {
-          Set<String> liveNodes = fetchLiveNodes(client);
-          this.liveNodes = (liveNodes);
-          liveNodesTimestamp = System.nanoTime();
-          return liveNodes;
-        } catch (Exception e) {
-          log.warn("Attempt to fetch cluster state from {} failed.", baseUrl, 
e);
-        }
-      }
+
+      if (updateLiveNodes(liveNodes)) return this.liveNodes;
+
+      log.warn(
+          "Attempt to fetch cluster state from all known live nodes {} failed. 
Trying backup nodes",
+          liveNodes);
+
+      if (updateLiveNodes(backupNodes)) return this.liveNodes;
+
       throw new RuntimeException(
           "Tried fetching live_nodes using all the node names we knew of, i.e. 
"
               + liveNodes

Review Comment:
   and the configured nodes.  Also don't recommend zkHosts :-)



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -226,17 +232,15 @@ public Set<String> getLiveNodes() {
     }

Review Comment:
   Just above this, there's a liveNodes null check.  That seems bogus now



##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -51,6 +55,7 @@ public abstract class BaseHttpClusterStateProvider implements 
ClusterStateProvid
   private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   private String urlScheme;
+  private Set<String> backupNodes;

Review Comment:
   Can we name this configuredNodes since that's what it is?  Yes we use it as 
a backup but we could adapt how we use them.



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

Reply via email to