madrob commented on code in PR #804:
URL: https://github.com/apache/solr/pull/804#discussion_r884977847


##########
solr/solrj/src/java/org/apache/solr/common/cloud/NodesSysPropsCacher.java:
##########
@@ -17,193 +17,87 @@
 
 package org.apache.solr.common.cloud;
 
-import static org.apache.solr.common.cloud.rule.ImplicitSnitch.SYSPROP;
-
-import java.lang.invoke.MethodHandles;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicInteger;
-import java.util.stream.Collectors;
-import org.apache.solr.client.solrj.cloud.NodeStateProvider;
-import org.apache.solr.client.solrj.routing.PreferenceRule;
-import org.apache.solr.common.SolrCloseable;
-import org.apache.solr.common.params.ShardParams;
-import org.apache.solr.common.util.CommonTestInjection;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-/**
- * Caching other nodes system properties. The properties that will be cached 
based on the value
- * define in {@link 
org.apache.solr.common.cloud.ZkStateReader#DEFAULT_SHARD_PREFERENCES } of {@link
- * org.apache.solr.common.cloud.ZkStateReader#CLUSTER_PROPS }. If that key 
does not present then
- * this cacher will do nothing.
- *
- * <p>The cache will be refresh whenever /live_nodes get changed.
- */
-public class NodesSysPropsCacher implements SolrCloseable {
-  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
-  private static final int NUM_RETRY = 5;
-
-  private final AtomicBoolean isRunning = new AtomicBoolean(false);
-  private final NodeStateProvider nodeStateProvider;
-  private Map<String, String> additionalProps = 
CommonTestInjection.injectAdditionalProps();
-  private final String currentNode;
-  private final ConcurrentHashMap<String, Map<String, Object>> cache = new 
ConcurrentHashMap<>();
-  private final AtomicInteger fetchCounting = new AtomicInteger(0);
-
-  private volatile boolean isClosed;
-  private volatile Collection<String> tags = new ArrayList<>();
-
-  public NodesSysPropsCacher(
-      NodeStateProvider nodeStateProvider, String currentNode, ZkStateReader 
stateReader) {
-    this.nodeStateProvider = nodeStateProvider;
-    this.currentNode = currentNode;
-
-    stateReader.registerClusterPropertiesListener(
-        properties -> {
-          Collection<String> tags = new ArrayList<>();
-          String shardPreferences =
-              (String) 
properties.getOrDefault(ZkStateReader.DEFAULT_SHARD_PREFERENCES, "");
-          if 
(shardPreferences.contains(ShardParams.SHARDS_PREFERENCE_NODE_WITH_SAME_SYSPROP))
 {
-            try {
-              tags =
-                  PreferenceRule.from(shardPreferences).stream()
-                      .filter(
-                          r -> 
ShardParams.SHARDS_PREFERENCE_NODE_WITH_SAME_SYSPROP.equals(r.name))
-                      .map(r -> r.value)
-                      .collect(Collectors.toSet());
-            } catch (Exception e) {
-              log.info("Error on parsing shards preference:{}", 
shardPreferences);
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.impl.CloudLegacySolrClient;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.common.NavigableObject;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.Utils;
+
+/** Fetch lazily and cache a node's system properties */
+public class NodesSysPropsCacher implements AutoCloseable {
+  private volatile boolean isClosed = false;
+  private final Map<String, Map<String, Object>> nodeVsTagsCache = new 
ConcurrentHashMap<>();
+  private ZkStateReader zkStateReader;
+  private final CloudLegacySolrClient solrClient;
+
+  public NodesSysPropsCacher(CloudLegacySolrClient solrClient, ZkStateReader 
zkStateReader) {
+    this.zkStateReader = zkStateReader;
+    this.solrClient = solrClient;
+    zkStateReader.registerLiveNodesListener(
+        (oldNodes, newNodes) -> {
+          for (String n : oldNodes) {
+            if (!newNodes.contains(n)) {
+              // this node has gone down, clear data
+              nodeVsTagsCache.remove(n);
             }
           }
-
-          if (tags.isEmpty()) {
-            pause();
-          } else {
-            start(tags);
-            // start fetching now
-            fetchSysProps(stateReader.getClusterState().getLiveNodes());
-          }
-          return isClosed;
-        });
-
-    stateReader.registerLiveNodesListener(
-        (oldLiveNodes, newLiveNodes) -> {
-          fetchSysProps(newLiveNodes);
           return isClosed;
         });
   }
 
-  private void start(Collection<String> tags) {
-    if (isClosed) return;
-    this.tags = tags;
-    isRunning.set(true);
-  }
-
-  private void fetchSysProps(Set<String> newLiveNodes) {
-    if (isRunning.get()) {
-      int fetchRound = fetchCounting.incrementAndGet();
-      // TODO smarter keeping caching entries by relying on Stat.cversion
-      cache.clear();
-      for (String node : newLiveNodes) {
-        // this might takes some times to finish, therefore if there are a 
latter change in listener
-        // triggering this method, skipping the old runner
-        if (isClosed && fetchRound != fetchCounting.get()) return;
-
-        if (currentNode.equals(node)) {
-          Map<String, String> props = new HashMap<>();
-          for (String tag : tags) {
-            String propName = tag.substring(SYSPROP.length());
-            if (additionalProps != null && 
additionalProps.containsKey(propName)) {
-              props.put(tag, additionalProps.get(propName));
-            } else {
-              props.put(tag, System.getProperty(propName));
-            }
-          }
-          cache.put(node, Collections.unmodifiableMap(props));
-        } else {
-          fetchRemoteProps(node, fetchRound);
-        }
-      }
-    }
-  }
-
-  private void fetchRemoteProps(String node, int fetchRound) {
-    for (int i = 0; i < NUM_RETRY; i++) {
-      if (isClosed && fetchRound != fetchCounting.get()) return;
-
-      try {
-        Map<String, Object> props = nodeStateProvider.getNodeValues(node, 
tags);
-        cache.put(node, Collections.unmodifiableMap(props));
-        break;
-      } catch (Exception e) {
-        try {
-          // 1, 4, 9
-          int backOffTime = 1000 * (i + 1);
-          backOffTime = backOffTime * backOffTime;
-          backOffTime = Math.min(10000, backOffTime);
-          Thread.sleep(backOffTime);
-        } catch (InterruptedException e1) {
-          Thread.currentThread().interrupt();
-          log.info(
-              "Exception on caching node:{} system.properties:{}, retry {}/{}",
-              node,
-              tags,
-              i + 1,
-              NUM_RETRY,
-              e); // nowarn
-          break;
-        }
-        log.info(
-            "Exception on caching node:{} system.properties:{}, retry {}/{}",
-            node,
-            tags,
-            i + 1,
-            NUM_RETRY,
-            e); // nowarn
-      }
-    }
-  }
-
-  public Map<String, Object> getSysProps(String node, Collection<String> tags) 
{
-    Map<String, Object> props = cache.get(node);
-    HashMap<String, Object> result = new HashMap<>();
-    if (props != null) {
-      for (String tag : tags) {
-        if (props.containsKey(tag)) {
-          result.put(tag, props.get(tag));
-        }
+  public Map<String, Object> getSysProps(String nodeName, Collection<String> 
tags) {
+    Map<String, Object> cached =
+        nodeVsTagsCache.computeIfAbsent(nodeName, s -> new LinkedHashMap<>());
+    Map<String, Object> result = new LinkedHashMap<>();
+    for (String tag : tags) {
+      if (!cached.containsKey(tag)) {
+        // at least one property is missing. fetch properties from the node
+        Map<String, Object> props = fetchProps(nodeName, tags);
+        // make a copy
+        cached = new LinkedHashMap<>(cached);
+        // merge all properties
+        cached.putAll(props);
+        // update the cache with the new set of properties
+        nodeVsTagsCache.put(nodeName, cached);
+        return props;
+      } else {
+        result.put(tag, cached.get(tag));
       }
     }
     return result;
   }
 
-  public int getCacheSize() {
-    return cache.size();
-  }
-
-  public boolean isRunning() {
-    return isRunning.get();
-  }
+  private Map<String, Object> fetchProps(String nodeName, Collection<String> 
tags) {
+    SolrParams p = new ModifiableSolrParams();
+    StringBuilder sb = new 
StringBuilder(zkStateReader.getBaseUrlForNodeName(nodeName));
+    sb.append("/admin/metrics?omitHeader=true&wt=javabin");

Review Comment:
   On the other hands, if we're using HTTP client because SolrClient can't 
handle these endpoints, then let's fix Solr Client to be better instead of 
trying to band aid around it



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