SOLR-17955: OTEL metrics: SplitShardCmd.checkDiskSpace needs conversion by mlbiscoc · Pull Request #3859 · apache/solr

mlbiscoc · 2025-11-12T20:59:36Z

https://issues.apache.org/jira/browse/SOLR-17955

Splitshard disk space check was broken when switching the /admin/metrics api to prometheus. This fixes that by reading the parsed prometheus output.

mlbiscoc · 2025-11-12T21:01:58Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

+    if (indexSize == -1.0) {
      log.warn("cannot verify information for parent shard leader");
      return;
    }
-    double indexSize = size.doubleValue();

-    Number freeSize =
-        (Number) rsp.getResponse()._get(List.of("metrics", freeDiskSpaceMetricName), null);
-    if (freeSize == null) {
+    if (freeSize == -1.0) {
      log.warn("missing node disk space information for parent shard leader");
      return;
    }


I'm thinking for here, this should throw instead of just return and continuing with the shard split cmd. Tests were passing because of this. Then we could at least know there was a regression. Was this just return for some specific purpose?

At least assert but I'm +1 to just throw if you wish.

As a general rule, never log AND throw. Pick one. Here, you choose to throw, so ditch the log. Logging is typically redundant with throwing, since throwing basically always gets logged as well.

dsmiley · 2025-11-14T04:50:27Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

+
+    SolrResponse resp = req.process(cloudManager.getSolrClient());
+
+    double[] sizes = new double[] {-1.0, -1.0}; // [indexSize, freeSize]


why an array?

It was kind of a work around solution for the lambda filter. I can move off of it if you think it's too messy.

it is too messy. In this case, use two AtomicDoubles

dsmiley · 2025-11-14T04:51:51Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

+    if (indexSize == -1.0) {
      log.warn("cannot verify information for parent shard leader");
      return;
    }
-    double indexSize = size.doubleValue();

-    Number freeSize =
-        (Number) rsp.getResponse()._get(List.of("metrics", freeDiskSpaceMetricName), null);
-    if (freeSize == null) {
+    if (freeSize == -1.0) {
      log.warn("missing node disk space information for parent shard leader");
      return;
    }


At least assert but I'm +1 to just throw if you wish.

dsmiley · 2025-11-14T04:52:47Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java

+            SolrRequest.METHOD.GET, "/admin/metrics", SolrRequest.SolrRequestType.ADMIN, params);
+    req.setResponseParser(new InputStreamResponseParser("prometheus"));
+
+    SolrResponse resp = req.process(cloudManager.getSolrClient());


this is going to talk to some arbitrary node, not necessarily the node that has the specific core we are going to split. To do that, you can get the URL from the Replica, truncate to the node, and use Http2SolrClient.requestWithBaseUrl

dsmiley

The exceptions make sense actually since Solr provides an option to disable the size check. So a user can disable them if they like.

Basically approving but a couple minor improvements. And needs a changelog.

dsmiley · 2025-12-09T16:45:49Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java


-    SolrResponse resp = req.process(cloudManager.getSolrClient());
+    var cloudClient = (CloudHttp2SolrClient) cloudManager.getSolrClient();
+    var http2Client = (Http2SolrClient) cloudClient.getHttpClient();


after you sync from main, please rename this var without the 2. IMO we shouldn't have this number 2 in our client var names

mlbiscoc · 2025-12-17T19:31:28Z

Made last few changes on 46149ca that I forgot to do.

dsmiley

Just a minor improvement request but LGTM otherwise.
Thanks for fixing two issues in one here :-)

dsmiley · 2025-12-18T02:07:25Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java


    var cloudClient = (CloudHttp2SolrClient) cloudManager.getSolrClient();
-    var http2Client = (Http2SolrClient) cloudClient.getHttpClient();
+    var httpClient = (HttpJettySolrClient) cloudClient.getHttpClient();


I suspect you no longer even need to cast... but whatever.

dsmiley · 2025-12-18T02:10:45Z

solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java


-    SolrResponse resp =
-        http2Client.requestWithBaseUrl(parentShardLeader.getBaseUrl(), req::process);
+    SolrResponse resp = httpClient.requestWithBaseUrl(parentShardLeader.getBaseUrl(), req::process);


I wouldn't be surprised if we deprecate the method you are calling. It's overloaded; the other one is defined on the base client, is used somewhat widely, and returns a NamedList. Looking at the code here, you only want a NamedList, not SolrResponse specifically.

dsmiley · 2025-12-18T02:11:44Z

changelog/unreleased/SOLR-17955-SplitShardCmd.checkDiskSpace-otel.yml

+  - name: Matthew Biscocho
+  - name: David Smiley
+links:
+  - name: SOLR-17955


maybe reference both JIRA issues that this addresses

I created another changelog entry instead

dsmiley · 2026-01-24T04:13:51Z

@mlbiscoc this is waiting for > a month; missed 10.0

mlbiscoc

this is waiting for > a month; missed 10.0

Oof sorry! completely forgot about this...

Merged in main to get this back up to date. Will merge by EOD unless you still have concerns.

mlbiscoc · 2026-01-26T17:31:00Z

changelog/unreleased/SOLR-17955-SplitShardCmd.checkDiskSpace-otel.yml

+  - name: Matthew Biscocho
+  - name: David Smiley
+links:
+  - name: SOLR-17955


I created another changelog entry instead

…eeds conversion (#3859) Fixes split shard which was disabled after moving metrics to OTEL. Split shard now parses and reads prometheus metrics for disk space on the correct node for the split shard command. Also a minor change to throw when failing to get the index size and free size metrics.

mlbiscoc and others added 2 commits November 12, 2025 15:16

Migrate splitshard check diskspace

29247fd

Update metric names

8aa5b44

mlbiscoc requested a review from dsmiley November 12, 2025 20:59

github-actions bot added cat:cloud cat:api labels Nov 12, 2025

mlbiscoc commented Nov 12, 2025

View reviewed changes

dsmiley reviewed Nov 14, 2025

View reviewed changes

Throw exceptions

62a82e0

dsmiley approved these changes Dec 9, 2025

View reviewed changes

Matthew Biscocho added 4 commits December 16, 2025 17:34

Merge branch 'main' into split-shard-disk

927a3d2

Use atomic reference

46149ca

Add changelog

dff6357

Add colon

1def5bb

dsmiley approved these changes Dec 18, 2025

View reviewed changes

mlbiscoc added 2 commits January 26, 2026 10:56

Merge branch 'main' into split-shard-disk

7317d1a

Add additional changelog for SOLR-17437

7f30ea0

mlbiscoc commented Jan 26, 2026

View reviewed changes

mlbiscoc merged commit 70d8caa into apache:main Jan 26, 2026
5 checks passed


		SolrResponse resp = req.process(cloudManager.getSolrClient());

		double[] sizes = new double[] {-1.0, -1.0}; // [indexSize, freeSize]

Comments

Conversation

mlbiscoc commented Nov 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbiscoc commented Dec 17, 2025

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Jan 24, 2026

Uh oh!

mlbiscoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants