dsmiley commented on code in PR #1625:
URL: https://github.com/apache/solr/pull/1625#discussion_r1185084634
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/MetricsFetcher.java:
##########
@@ -0,0 +1,202 @@
+package org.apache.solr.client.solrj.impl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.solr.client.solrj.response.SimpleSolrResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+
+// uses metrics API to get node information
+public class MetricsFetcher {
+ // well known tags
+ public static final String NODE = "node";
+ public static final String PORT = "port";
+ public static final String HOST = "host";
+ public static final String CORES = "cores";
+ public static final String DISK = "freedisk";
+ public static final String ROLE = "role";
+ public static final String NODEROLE = "nodeRole";
+ public static final String SYSPROP = "sysprop.";
+ public static final String SYSLOADAVG = "sysLoadAvg";
+ public static final String HEAPUSAGE = "heapUsage";
+ public static final Set<String> tags = Set.of(NODE, PORT, HOST, CORES, DISK,
ROLE, HEAPUSAGE);
+ public static final Pattern hostAndPortPattern =
Pattern.compile("(?:https?://)?([^:]+):(\\d+)");
+ public static final String METRICS_PREFIX = "metrics:";
+
+ protected void getRemoteInfo(
+ String solrNode, Set<String> requestedTags,
SolrClientNodeStateProvider.RemoteCallCtx ctx) {
+ if (!(ctx).isNodeAlive(solrNode)) return;
+ Map<String, Set<Object>> metricsKeyVsTag = new HashMap<>();
+ for (String tag : requestedTags) {
+ if (tag.startsWith(SYSPROP)) {
+ metricsKeyVsTag
+ .computeIfAbsent(
+ "solr.jvm:system.properties:" +
tag.substring(SYSPROP.length()),
+ k -> new HashSet<>())
+ .add(tag);
+ } else if (tag.startsWith(METRICS_PREFIX)) {
+ metricsKeyVsTag
+ .computeIfAbsent(tag.substring(METRICS_PREFIX.length()), k -> new
HashSet<>())
+ .add(tag);
+ }
+ }
+ if (!metricsKeyVsTag.isEmpty()) {
+ SolrClientNodeStateProvider.fetchReplicaMetrics(solrNode, ctx,
metricsKeyVsTag);
+ }
+
+ Set<String> groups = new HashSet<>();
+ List<String> prefixes = new ArrayList<>();
+ if (requestedTags.contains(DISK)) {
+ groups.add("solr.node");
+ prefixes.add("CONTAINER.fs.usableSpace");
+ }
+ if (requestedTags.contains(Variable.TOTALDISK.tagName)) {
+ groups.add("solr.node");
+ prefixes.add("CONTAINER.fs.totalSpace");
+ }
+ if (requestedTags.contains(CORES)) {
+ groups.add("solr.node");
+ prefixes.add("CONTAINER.cores");
+ }
+ if (requestedTags.contains(SYSLOADAVG)) {
+ groups.add("solr.jvm");
+ prefixes.add("os.systemLoadAverage");
+ }
+ if (requestedTags.contains(HEAPUSAGE)) {
+ groups.add("solr.jvm");
+ prefixes.add("memory.heap.usage");
+ }
+ if (groups.isEmpty() || prefixes.isEmpty()) return;
+
+ ModifiableSolrParams params = new ModifiableSolrParams();
+ params.add("group", StrUtils.join(groups, ','));
+ params.add("prefix", StrUtils.join(prefixes, ','));
+
+ try {
+ SimpleSolrResponse rsp = ctx.invokeWithRetry(solrNode,
CommonParams.METRICS_PATH, params);
+ NamedList<?> metrics = (NamedList<?>) rsp.nl.get("metrics");
+ if (metrics != null) {
+ // metrics enabled
+ if (requestedTags.contains(Variable.FREEDISK.tagName)) {
+ Object n = Utils.getObjectByPath(metrics, true,
"solr.node/CONTAINER.fs.usableSpace");
+ if (n != null) ctx.tags.put(Variable.FREEDISK.tagName,
Variable.FREEDISK.convertVal(n));
+ }
+ if (requestedTags.contains(Variable.TOTALDISK.tagName)) {
+ Object n = Utils.getObjectByPath(metrics, true,
"solr.node/CONTAINER.fs.totalSpace");
+ if (n != null) ctx.tags.put(Variable.TOTALDISK.tagName,
Variable.TOTALDISK.convertVal(n));
+ }
+ if (requestedTags.contains(CORES)) {
+ NamedList<?> node = (NamedList<?>) metrics.get("solr.node");
+ int count = 0;
+ for (String leafCoreMetricName : new String[] {"lazy", "loaded",
"unloaded"}) {
+ Number n = (Number) node.get("CONTAINER.cores." +
leafCoreMetricName);
+ if (n != null) count += n.intValue();
+ }
+ ctx.tags.put(CORES, count);
+ }
+ if (requestedTags.contains(SYSLOADAVG)) {
+ Number n = (Number) Utils.getObjectByPath(metrics, true,
"solr.jvm/os.systemLoadAverage");
+ if (n != null) ctx.tags.put(SYSLOADAVG, n.doubleValue());
+ }
+ if (requestedTags.contains(HEAPUSAGE)) {
+ Number n = (Number) Utils.getObjectByPath(metrics, true,
"solr.jvm/memory.heap.usage");
+ if (n != null) ctx.tags.put(HEAPUSAGE, n.doubleValue() * 100.0d);
+ }
+ }
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error
getting remote info", e);
+ }
+ }
+
+ public void getTags(
+ String solrNode, Set<String> requestedTags,
SolrClientNodeStateProvider.RemoteCallCtx ctx) {
+ try {
+ if (requestedTags.contains(NODE)) ctx.tags.put(NODE, solrNode);
+ if (requestedTags.contains(HOST)) {
+ Matcher hostAndPortMatcher = hostAndPortPattern.matcher(solrNode);
+ if (hostAndPortMatcher.find()) ctx.tags.put(HOST,
hostAndPortMatcher.group(1));
+ }
+ if (requestedTags.contains(PORT)) {
+ Matcher hostAndPortMatcher = hostAndPortPattern.matcher(solrNode);
+ if (hostAndPortMatcher.find()) ctx.tags.put(PORT,
hostAndPortMatcher.group(2));
+ }
+ if (requestedTags.contains(ROLE)) fillRole(solrNode, ctx, ROLE);
+ if (requestedTags.contains(NODEROLE))
+ fillRole(solrNode, ctx, NODEROLE); // for new policy framework
+
+ getRemoteInfo(solrNode, requestedTags, ctx);
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
+ }
+ }
+
+ private void fillRole(String solrNode,
SolrClientNodeStateProvider.RemoteCallCtx ctx, String key)
+ throws KeeperException, InterruptedException {
+ Map<?, ?> roles =
+ (Map<?, ?>)
+ (ctx.session != null
+ ? ctx.session.get(ZkStateReader.ROLES)
+ : null); // we don't want to hit the ZK for each node
+ // so cache and reuse
+ try {
+ if (roles == null) roles = ctx.getZkJson(ZkStateReader.ROLES);
+ cacheRoles(solrNode, ctx, key, roles);
+ } catch (KeeperException.NoNodeException e) {
+ cacheRoles(solrNode, ctx, key, Collections.emptyMap());
+ }
+ }
+
+ private void cacheRoles(
+ String solrNode, SolrClientNodeStateProvider.RemoteCallCtx ctx, String
key, Map<?, ?> roles) {
+ if (ctx.session != null) ctx.session.put(ZkStateReader.ROLES, roles);
+ if (roles != null) {
+ for (Map.Entry<?, ?> e : roles.entrySet()) {
+ if (e.getValue() instanceof List) {
+ if (((List<?>) e.getValue()).contains(solrNode)) {
+ ctx.tags.put(key, e.getKey());
+ break;
+ }
+ }
+ }
+ }
+ }
+
+ public enum Variable {
Review Comment:
At least a one line javadoc on what this is. Seems similar to the other
"tags" although the value varies? Say that.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/MetricsFetcher.java:
##########
@@ -0,0 +1,202 @@
+package org.apache.solr.client.solrj.impl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.solr.client.solrj.response.SimpleSolrResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+
+// uses metrics API to get node information
Review Comment:
Javadocs please. If it was written for something in particular in mind,
then it's very useful to say that, even if it may theoretically have future
uses we don't know about. It's not clear why would something use this instead
of going directly to the Metrics API? As a newcomer to this code, is it
related to having short/simply well-known named "tags" (formerly maybe called
snitch?) that loosely correlate to a more complex metrics expression, so that
some other code can use these tags to make node placement decisions based on
those same tags?
The lifecycle of an instance of this class is not obvious; please add a
sentence on that.
##########
solr/core/src/java/org/apache/solr/cluster/placement/impl/AttributeFetcherImpl.java:
##########
@@ -234,20 +233,20 @@ private static SolrInfoBean.Group
getGroupFromMetricRegistry(NodeMetric.Registry
public static String getMetricSnitchTag(NodeMetric<?> metric) {
Review Comment:
Assuming an objective is to remove the word "Snitch" everywhere, then this
method name should change.
And would like to see a javadoc on this method if you can to show an example.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/MetricsFetcher.java:
##########
@@ -0,0 +1,202 @@
+package org.apache.solr.client.solrj.impl;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.solr.client.solrj.response.SimpleSolrResponse;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.zookeeper.KeeperException;
+
+// uses metrics API to get node information
+public class MetricsFetcher {
+ // well known tags
+ public static final String NODE = "node";
+ public static final String PORT = "port";
+ public static final String HOST = "host";
+ public static final String CORES = "cores";
+ public static final String DISK = "freedisk";
+ public static final String ROLE = "role";
+ public static final String NODEROLE = "nodeRole";
+ public static final String SYSPROP = "sysprop.";
+ public static final String SYSLOADAVG = "sysLoadAvg";
+ public static final String HEAPUSAGE = "heapUsage";
Review Comment:
Should be an enum?
If you keep it, then I'd add a newline below to better delineate the tags
with other constants
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
