You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2023/03/23 14:34:03 UTC
[GitHub] [solr] dsmiley commented on a diff in pull request #1485: SOLR-16507: Remove NodeStateProvider & Snitch
dsmiley commented on code in PR #1485:
URL: https://github.com/apache/solr/pull/1485#discussion_r1146200128
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -873,42 +873,85 @@ public static void checkDiskSpace(
SolrIndexSplitter.SplitMethod method,
SolrCloudManager cloudManager)
throws SolrException {
+
// check that enough disk space is available on the parent leader node
// otherwise the actual index splitting will always fail
- NodeStateProvider nodeStateProvider = cloudManager.getNodeStateProvider();
- Map<String, Object> nodeValues =
- nodeStateProvider.getNodeValues(
- parentShardLeader.getNodeName(), Collections.singletonList(ImplicitSnitch.DISK));
- Map<String, Map<String, List<Replica>>> infos =
- nodeStateProvider.getReplicaInfo(
- parentShardLeader.getNodeName(), Collections.singletonList(CORE_IDX.metricsAttribute));
- if (infos.get(collection) == null || infos.get(collection).get(shard) == null) {
+
+ ModifiableSolrParams params;
+ String metricName;
+ GenericSolrRequest req;
+ SolrResponse rsp;
+
+ metricName =
+ new StringBuilder("solr.core.")
Review Comment:
Needless use of StringBuilder; just use simple string concatenation.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -873,42 +873,85 @@ public static void checkDiskSpace(
SolrIndexSplitter.SplitMethod method,
SolrCloudManager cloudManager)
throws SolrException {
+
// check that enough disk space is available on the parent leader node
// otherwise the actual index splitting will always fail
- NodeStateProvider nodeStateProvider = cloudManager.getNodeStateProvider();
- Map<String, Object> nodeValues =
- nodeStateProvider.getNodeValues(
- parentShardLeader.getNodeName(), Collections.singletonList(ImplicitSnitch.DISK));
- Map<String, Map<String, List<Replica>>> infos =
- nodeStateProvider.getReplicaInfo(
- parentShardLeader.getNodeName(), Collections.singletonList(CORE_IDX.metricsAttribute));
- if (infos.get(collection) == null || infos.get(collection).get(shard) == null) {
+
+ ModifiableSolrParams params;
+ String metricName;
+ GenericSolrRequest req;
+ SolrResponse rsp;
Review Comment:
This is a coding style that I have only ever seen decades ago in languages like "C". Please don't declare a bunch of variables up front; declare them at the latest possible time, and with its initial value if possible.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -873,42 +873,85 @@ public static void checkDiskSpace(
SolrIndexSplitter.SplitMethod method,
SolrCloudManager cloudManager)
throws SolrException {
+
// check that enough disk space is available on the parent leader node
// otherwise the actual index splitting will always fail
- NodeStateProvider nodeStateProvider = cloudManager.getNodeStateProvider();
- Map<String, Object> nodeValues =
- nodeStateProvider.getNodeValues(
- parentShardLeader.getNodeName(), Collections.singletonList(ImplicitSnitch.DISK));
- Map<String, Map<String, List<Replica>>> infos =
- nodeStateProvider.getReplicaInfo(
- parentShardLeader.getNodeName(), Collections.singletonList(CORE_IDX.metricsAttribute));
- if (infos.get(collection) == null || infos.get(collection).get(shard) == null) {
+
+ ModifiableSolrParams params;
+ String metricName;
+ GenericSolrRequest req;
+ SolrResponse rsp;
+
+ metricName =
+ new StringBuilder("solr.core.")
+ .append(collection)
+ .append(".")
+ .append(shard)
+ .append(".")
+ .append(Utils.parseMetricsReplicaName(collection, parentShardLeader.getCoreName()))
+ .append(":INDEX.sizeInBytes")
+ .toString();
+
+ params = new ModifiableSolrParams();
+ params.add("key", metricName);
+
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/metrics", params);
+ try {
+ rsp = cloudManager.request(req);
+ } catch (Exception e) {
+ log.error("Error occurred while checking the disk space of the node");
+ return;
+ }
+
+ if (rsp.getResponse() == null) {
log.warn("cannot verify information for parent shard leader");
return;
}
- // find the leader
- List<Replica> lst = infos.get(collection).get(shard);
- Double indexSize = null;
- for (Replica info : lst) {
- if (info.getCoreName().equals(parentShardLeader.getCoreName())) {
- Number size = (Number) info.get(CORE_IDX.metricsAttribute);
- if (size == null) {
- log.warn("cannot verify information for parent shard leader");
- return;
- }
- indexSize = (Double) CORE_IDX.convertVal(size);
- break;
- }
+
+ NamedList<Object> response = rsp.getResponse();
+ Object value = response.findRecursive("metrics", metricName);
+ if (value == null) {
+ log.warn("cannot verify information for parent shard leader");
+ return;
+ }
+
+ Number size = (Number) value;
+ if (size == null) {
+ log.warn("cannot verify information for parent shard leader");
+ return;
}
- if (indexSize == null) {
- log.warn("missing replica information for parent shard leader");
+ double indexSize = size.doubleValue();
+
+ metricName = "solr.node:CONTAINER.fs.usableSpace";
+ params = new ModifiableSolrParams();
+ params.add("key", metricName);
Review Comment:
BTW can do all in one line:
`SolrParams = new ModifiableSolrParams().add("key", metricName);`
Please avoid code verbosity.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -873,42 +873,85 @@ public static void checkDiskSpace(
SolrIndexSplitter.SplitMethod method,
SolrCloudManager cloudManager)
throws SolrException {
+
// check that enough disk space is available on the parent leader node
// otherwise the actual index splitting will always fail
- NodeStateProvider nodeStateProvider = cloudManager.getNodeStateProvider();
- Map<String, Object> nodeValues =
- nodeStateProvider.getNodeValues(
- parentShardLeader.getNodeName(), Collections.singletonList(ImplicitSnitch.DISK));
- Map<String, Map<String, List<Replica>>> infos =
- nodeStateProvider.getReplicaInfo(
- parentShardLeader.getNodeName(), Collections.singletonList(CORE_IDX.metricsAttribute));
- if (infos.get(collection) == null || infos.get(collection).get(shard) == null) {
+
+ ModifiableSolrParams params;
+ String metricName;
+ GenericSolrRequest req;
+ SolrResponse rsp;
+
+ metricName =
+ new StringBuilder("solr.core.")
+ .append(collection)
+ .append(".")
+ .append(shard)
+ .append(".")
+ .append(Utils.parseMetricsReplicaName(collection, parentShardLeader.getCoreName()))
+ .append(":INDEX.sizeInBytes")
+ .toString();
+
+ params = new ModifiableSolrParams();
+ params.add("key", metricName);
+
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/metrics", params);
+ try {
+ rsp = cloudManager.request(req);
+ } catch (Exception e) {
+ log.error("Error occurred while checking the disk space of the node");
+ return;
+ }
+
+ if (rsp.getResponse() == null) {
log.warn("cannot verify information for parent shard leader");
return;
}
- // find the leader
- List<Replica> lst = infos.get(collection).get(shard);
- Double indexSize = null;
- for (Replica info : lst) {
- if (info.getCoreName().equals(parentShardLeader.getCoreName())) {
- Number size = (Number) info.get(CORE_IDX.metricsAttribute);
- if (size == null) {
- log.warn("cannot verify information for parent shard leader");
- return;
- }
- indexSize = (Double) CORE_IDX.convertVal(size);
- break;
- }
+
+ NamedList<Object> response = rsp.getResponse();
+ Object value = response.findRecursive("metrics", metricName);
+ if (value == null) {
+ log.warn("cannot verify information for parent shard leader");
+ return;
+ }
+
Review Comment:
Not needed; don't declare "value"
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -873,42 +873,85 @@ public static void checkDiskSpace(
SolrIndexSplitter.SplitMethod method,
SolrCloudManager cloudManager)
throws SolrException {
+
// check that enough disk space is available on the parent leader node
// otherwise the actual index splitting will always fail
- NodeStateProvider nodeStateProvider = cloudManager.getNodeStateProvider();
- Map<String, Object> nodeValues =
- nodeStateProvider.getNodeValues(
- parentShardLeader.getNodeName(), Collections.singletonList(ImplicitSnitch.DISK));
- Map<String, Map<String, List<Replica>>> infos =
- nodeStateProvider.getReplicaInfo(
- parentShardLeader.getNodeName(), Collections.singletonList(CORE_IDX.metricsAttribute));
- if (infos.get(collection) == null || infos.get(collection).get(shard) == null) {
+
+ ModifiableSolrParams params;
+ String metricName;
+ GenericSolrRequest req;
+ SolrResponse rsp;
+
+ metricName =
+ new StringBuilder("solr.core.")
+ .append(collection)
+ .append(".")
+ .append(shard)
+ .append(".")
+ .append(Utils.parseMetricsReplicaName(collection, parentShardLeader.getCoreName()))
+ .append(":INDEX.sizeInBytes")
+ .toString();
+
+ params = new ModifiableSolrParams();
+ params.add("key", metricName);
+
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/metrics", params);
+ try {
+ rsp = cloudManager.request(req);
+ } catch (Exception e) {
+ log.error("Error occurred while checking the disk space of the node");
+ return;
+ }
+
+ if (rsp.getResponse() == null) {
log.warn("cannot verify information for parent shard leader");
return;
}
- // find the leader
- List<Replica> lst = infos.get(collection).get(shard);
- Double indexSize = null;
- for (Replica info : lst) {
- if (info.getCoreName().equals(parentShardLeader.getCoreName())) {
- Number size = (Number) info.get(CORE_IDX.metricsAttribute);
- if (size == null) {
- log.warn("cannot verify information for parent shard leader");
- return;
- }
- indexSize = (Double) CORE_IDX.convertVal(size);
- break;
- }
+
+ NamedList<Object> response = rsp.getResponse();
+ Object value = response.findRecursive("metrics", metricName);
+ if (value == null) {
+ log.warn("cannot verify information for parent shard leader");
+ return;
+ }
+
+ Number size = (Number) value;
+ if (size == null) {
+ log.warn("cannot verify information for parent shard leader");
+ return;
}
- if (indexSize == null) {
- log.warn("missing replica information for parent shard leader");
+ double indexSize = size.doubleValue();
+
+ metricName = "solr.node:CONTAINER.fs.usableSpace";
+ params = new ModifiableSolrParams();
+ params.add("key", metricName);
+
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/metrics", params);
Review Comment:
You are doing a second request but the metrics API is rich enough to support both needs in one request. "key" can be provided multiple times for each value needed.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java:
##########
@@ -873,42 +873,85 @@ public static void checkDiskSpace(
SolrIndexSplitter.SplitMethod method,
SolrCloudManager cloudManager)
throws SolrException {
+
// check that enough disk space is available on the parent leader node
// otherwise the actual index splitting will always fail
- NodeStateProvider nodeStateProvider = cloudManager.getNodeStateProvider();
- Map<String, Object> nodeValues =
- nodeStateProvider.getNodeValues(
- parentShardLeader.getNodeName(), Collections.singletonList(ImplicitSnitch.DISK));
- Map<String, Map<String, List<Replica>>> infos =
- nodeStateProvider.getReplicaInfo(
- parentShardLeader.getNodeName(), Collections.singletonList(CORE_IDX.metricsAttribute));
- if (infos.get(collection) == null || infos.get(collection).get(shard) == null) {
+
+ ModifiableSolrParams params;
+ String metricName;
+ GenericSolrRequest req;
+ SolrResponse rsp;
+
+ metricName =
+ new StringBuilder("solr.core.")
+ .append(collection)
+ .append(".")
+ .append(shard)
+ .append(".")
+ .append(Utils.parseMetricsReplicaName(collection, parentShardLeader.getCoreName()))
+ .append(":INDEX.sizeInBytes")
+ .toString();
+
+ params = new ModifiableSolrParams();
+ params.add("key", metricName);
+
+ req = new GenericSolrRequest(SolrRequest.METHOD.GET, "/admin/metrics", params);
Review Comment:
could inline into below
--
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