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: [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]
