SOLR-15751: Modify v2 `GET /collections/collName` to support full ColStatus response #2912

gerlowskija · 2024-12-17T15:59:14Z

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

Description

Solr's v1 "/admin/collections?action=COLSTATUS" API is a useful way to get a high level summary of a single collection. This includes the collection status and metadata (which CLUSTERSTATUS also returns) as well as very detailed data core-level data about fields, disk-usage, etc.

The v2 API lacks all of this functionality.

Solution

This PR addresses this by modifying the existing v2 GET /collections/collName API to incorporate the v1 "colstatus" functionality. (Previously this v2 endpoint returned the same response as v1's /admin/collections?action=CLUSTERSTATUS&collection=collName`)

The PR also converts this endpoint to JAX-RS in the process, so that it's included in our OAS and codegen.

Tests

Added test in CollectionsAPISolrJTest that makes use of the OAS-generated SolrJ request/response. Existing tests continue to pass.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

TODO - test failures: TestDistributedTracing, TestApiFramework

epugh · 2024-12-17T16:03:00Z

If you think there are parameters that maybe don't make sense going forward, I would vote for skipping them. Get this in, and then see if folks highlight legit use cases for why they need those. If you blindly carry them forward, well, then, we probably are stuck with them forever.... It's easier to add them later then it is to remove them... ;-). Also, we should think about YAGNI more.

gerlowskija · 2024-12-17T16:17:23Z

If you blindly carry them forward, well, then, we probably are stuck with them forever

Definitely don't want that. But I also don't have too much trust in my very quick-and-dirty manual testing -- that's really all I have saying that the params don't do much. Gonna do more spelunking and then we can figure out what direction to go in.

dsmiley

Thanks for this

solr/api/src/java/org/apache/solr/client/api/model/CollectionStatusResponse.java

solr/core/src/java/org/apache/solr/api/V2HttpCall.java

solr/api/src/java/org/apache/solr/client/api/model/CollectionStatusResponse.java

dsmiley · 2024-12-17T17:13:07Z

solr/core/src/java/org/apache/solr/handler/admin/api/CollectionStatus.java

+    // TODO Push CollectionStatusResponse down into ColStatus and avoid the intermediate NL, if all
+    // usages can be switched over.
+    final var nlResponse = new NamedList<>();
+    new ColStatus(
+            coreContainer.getSolrClientCache(),
+            coreContainer.getZkController().getZkStateReader().getClusterState(),
+            new ZkNodeProps(params))
+        .getColStatus(nlResponse);
+    // collName is prop/key for a nested NL returned by ColStatus - extract the inner NL and
+    // manually add name to resp


so much for v1 moving to be on top of v2; ehh?

That model works great anytime that the v1 and v2 responses are exactly the same (which is almost always the case). I made the v2 response for this API slightly different from v1 COLSTATUS, so it's hard to have CollectionsHandler call V2ApiUtils.squashIntoNamedList(new CollectionStatus(..).getCollectionStatus(...)) the way we've been doing with most of our other v1-v2 pairs.

That said, I did a bit of restructuring here so that v1 can mostly sit on top of v2: CollectionsHandler.COLSTATUS now calls a static method populateColStatusData that lives in this class. Hopefully that gets us most of the benefit. If we eventually tackle the TODO comment at L87, we should be able to close the gap and have CollectionsHandler.COLSTATUS consume the v2 API in the normal way.

gerlowskija · 2024-12-17T20:16:25Z

re: impotent COLSTATUS boolean flags previously discussed here

I think I understand this a bit better now. The ref-guide docs for COLSTATUS describe each of the boolean flags as being independent of one another: segments returns base segment data, fieldInfo returns detailed information about each indexed field, etc. But in reality on main/branch_9x currently, the flags are dependent on one another. fieldInfo and sizeInfo have no impact unless paired with segments=true. And segments=true has no impact on its own either.

The ref-guide coverage here doesn't suggest that this is intentional in any way, so I suspect this is a bug that eluded @sigram when he first did SOLR-13292. (Hopefully AB can confirm his intentions if he catches this notification!)

Working on that theory, I'll bundle a fix for COLSTATUS into this PR and add some extra tests if there aren't any objections?

ColStatus was failing to check the value of the very-relevant 'segments' parameter when determining whether to make /admin/segments calls to each shard leader.

gerlowskija · 2024-12-18T20:23:01Z

Alright - COLSTATUS bug if fixed (see comment above), added some additional tests for both v1 and v2, and tests+check pass for me locally! Will aim to merge this in a few days pending any more feedback

dsmiley · 2024-12-18T22:05:13Z

solr/api/src/java/org/apache/solr/client/api/model/CollectionStatusResponse.java

+    @JsonProperty public Long sizeInBytes;
+    @JsonProperty public Integer size;
+    // A date string of the form "2024-12-17T17:35:18.275Z"
+    @JsonProperty public String age;


Can we use Instant?

I've used Date for now based on PR discussion, see SOLR-17608.

dsmiley · 2024-12-18T22:14:18Z

solr/api/src/java/org/apache/solr/client/api/model/CollectionStatusResponse.java

+
+  @JsonProperty public String name;
+  @JsonProperty public Integer znodeVersion;
+  @JsonProperty public Long creationTimeMillis;


Use Instant and drop "Millis"?

We can change the variable name and probably the type on the deserialized-Java side.

But we'll need to keep the same serialization-time prop name and value to avoid backcompat issues.

Will Date work in the interim?

Yep, Date can work until SOLR-17608 gets sorted out. If we think "Instant" is generally better, I can switch to 'Date', but add in a TODO comment with the JIRA ticket, so we can find this later on...

solr/api/src/java/org/apache/solr/client/api/model/CollectionStatusResponse.java

gerlowskija · 2024-12-19T18:53:53Z

Will aim to merge this in a few days pending any more feedback

Actually - thinking on it a bit - the v1 /admin/segments API is used under the hood by COLSTATUS, and has a very similar response format, so it probably makes sense to create a v2 equivalent for that API within this PR as well. I'll look to add that shortly.

gerlowskija · 2024-12-20T21:02:08Z

Alright - added the /admin/segments v2 API, so this is back in "almost ready" territory.

Still need to look into David's question about use of 'Instant' - I fiddled a bit and got it to work with Date, so hopefully it's do-able in at least some places.

gerlowskija · 2024-12-21T12:57:47Z

Still need to look into David's question about use of Instant

Alright - the main hiccup for using Instant is that it's not supported by our serialization layer. Jackson supports it pretty easily on the v2 side, but our v1 "ResponseWriter"-based serialization code doesn't. (The v1 code needs support because of how our v1 APIs are built "on top of" the v2 APIs - "primitive" types used in the v2 POJO end up in the NamedList that the v1 code serializes out.)

Anyway, I think it would be nice to support this, but I don't want to squeeze it into this PR so I'm not tempted to skimp on testing, etc. I've filed a ticket for it, and hope to pursue that shortly. In the meantime I added 'TODO' comments to the places David flagged as being 'Instant'-ready - we can circle back to those either as a part of SOLR-17608, or once it's done.

(FYI @dsmiley )

dsmiley · 2024-12-22T16:59:15Z

solr/api/src/java/org/apache/solr/client/api/endpoint/SegmentsApi.java

It's a shame to repeat this stuff. Is it possible to maybe subclass the API so that the CollectionStatusApi inherits SegmentsApi? If the only gotcha is a desire to document that some booleans require others then I'd rather we simply have the API return a 400 and tell the user that if you enable X then you must also enable Y. Quite fair & helpful IMO.

Fwiw, we've already got some reuse here: e.g. these SegmentsApi constants get re-used in CollectionStatusApi as well.

The main thing that prevents us from doing something more with inheritance, etc. is that the method signatures for CollectionStatusApi/SegmentsApi aren't the same. The CollectionStatusApi method takes two parameters ('collectionName' and 'segments') that have no SegmentApi equivalent.

dsmiley · 2024-12-22T17:00:16Z

solr/api/src/java/org/apache/solr/client/api/model/CollectionStatusResponse.java

+
+  @JsonProperty public String name;
+  @JsonProperty public Integer znodeVersion;
+  @JsonProperty public Long creationTimeMillis;


Will Date work in the interim?

dsmiley · 2024-12-22T18:33:02Z

solr/core/src/java/org/apache/solr/handler/admin/ColStatus.java

+              for (Map.Entry<String, GetSegmentDataResponse.SingleSegmentData> entry :
+                  segs.entrySet()) {
+                final var fieldInfoByName = entry.getValue().fields;


I love how typed this is now. This is an important part of the V2 vision coming into reality.

dsmiley · 2024-12-22T18:34:01Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetSegmentData.java

+  private static final double GB = 1024.0 * 1024.0 * 1024.0;
+
+  private static final List<String> FI_LEGEND =
+      Arrays.asList(


please avoid asList in favor of the immutable List.of

This stuff actually pre-exists this PR; it previously lived in SegmentInfoRequestHandler.

I generally try to avoid unrelated cleanups as I go through these v2 PRs. Partly for scope (if I stopped to fix every little thing, forward progress would halt entirely). And partly because you never know what seemingly "safe" change will end up biting you.

dsmiley · 2024-12-22T18:40:21Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetSegmentData.java

+                    if (p1.second() > p2.second()) {
+                      return -1;
+                    } else if (p1.second() < p2.second()) {
+                      return 1;
+                    } else {
+                      return 0;
+                    }


I think this can be replaced by usage of java.util.Comparator#comparing(java.util.function.Function<? super T,? extends U>)

[0] This stuff actually pre-exists this PR; it previously lived in SegmentInfoRequestHandler.

I generally try to avoid unrelated cleanups as I go through these v2 PRs. Partly for scope (if I stopped to fix every little thing, forward progress would halt entirely). And partly because you never know what seemingly "safe" change will end up biting you.

dsmiley · 2024-12-22T18:53:56Z

solr/core/src/test/org/apache/solr/handler/admin/IndexSizeEstimatorTest.java

-      @SuppressWarnings({"unchecked"})
-      Map<String, Object> rawSizeMap = rawSize.asMap(10);
-      @SuppressWarnings({"unchecked"})
-      Map<String, Object> fieldsBySize =
-          (Map<String, Object>) rawSizeMap.get(IndexSizeEstimator.FIELDS_BY_SIZE);
+      Map<String, String> fieldsBySize = rawSize.fieldsBySize;


again; so much better

gerlowskija added 4 commits December 14, 2024 15:59

Migrate coll-status API to JAX-RS

cba044d

TODO - test failures: TestDistributedTracing, TestApiFramework

Fix Two test failures

7b3198c

Align on v1 COLSTATUS response

205dfac

Document parameters

517082c

github-actions bot added tests cat:cloud cat:api labels Dec 17, 2024

Add ref-guide docs

6b3c14a

github-actions bot added the documentation Improvements or additions to documentation label Dec 17, 2024

dsmiley reviewed Dec 17, 2024

View reviewed changes

gerlowskija added 3 commits December 18, 2024 09:33

Fix COLSTATUS bug

f7bc066

ColStatus was failing to check the value of the very-relevant 'segments' parameter when determining whether to make /admin/segments calls to each shard leader.

Update CollectionStatusResponse, post COLSTATUS bug-fix

e06c21a

Address review comments, rd 1

d0117d2

gerlowskija changed the title ~~SOLR-15781: Modify v2 GET /collections/collName to support full ColStatus response~~ SOLR-15751: Modify v2 GET /collections/collName to support full ColStatus response Dec 18, 2024

dsmiley reviewed Dec 18, 2024

View reviewed changes

gerlowskija added 2 commits December 18, 2024 22:59

WIP: EOD cutoff for segmentsApi

933a242

WIP - save for machine switching

bbdcb24

gerlowskija added 3 commits December 20, 2024 11:58

Initial finished pass at /admin/segments JAX-RS

25cc599

Fix tests, pt 1

9b8852a

Fix tests, pt 2

b2c1690

Add TODO comments for SOLR-17608

639b8ea

dsmiley reviewed Dec 22, 2024

View reviewed changes

Switch to (temporary) use of 'Date' type where possible

c4103d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15751: Modify v2 `GET /collections/collName` to support full ColStatus response #2912

SOLR-15751: Modify v2 `GET /collections/collName` to support full ColStatus response #2912

gerlowskija commented Dec 17, 2024 •

edited

Loading

epugh commented Dec 17, 2024

gerlowskija commented Dec 17, 2024

dsmiley left a comment

dsmiley Dec 17, 2024

gerlowskija Dec 18, 2024

gerlowskija commented Dec 17, 2024

gerlowskija commented Dec 18, 2024

dsmiley Dec 18, 2024

gerlowskija Dec 23, 2024

dsmiley Dec 18, 2024

gerlowskija Dec 20, 2024

dsmiley Dec 22, 2024

gerlowskija Dec 23, 2024

gerlowskija commented Dec 19, 2024 •

edited

Loading

gerlowskija commented Dec 20, 2024

gerlowskija commented Dec 21, 2024

dsmiley Dec 22, 2024

gerlowskija Dec 23, 2024

dsmiley Dec 22, 2024

dsmiley Dec 22, 2024

dsmiley Dec 22, 2024

gerlowskija Dec 23, 2024 •

edited

Loading

dsmiley Dec 22, 2024

gerlowskija Dec 23, 2024

dsmiley Dec 22, 2024

SOLR-15751: Modify v2 GET /collections/collName to support full ColStatus response #2912

Are you sure you want to change the base?

SOLR-15751: Modify v2 GET /collections/collName to support full ColStatus response #2912

Conversation

gerlowskija commented Dec 17, 2024 • edited Loading

Description

Solution

Tests

Checklist

epugh commented Dec 17, 2024

gerlowskija commented Dec 17, 2024

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija commented Dec 17, 2024

gerlowskija commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija commented Dec 19, 2024 • edited Loading

gerlowskija commented Dec 20, 2024

gerlowskija commented Dec 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SOLR-15751: Modify v2 `GET /collections/collName` to support full ColStatus response #2912

SOLR-15751: Modify v2 `GET /collections/collName` to support full ColStatus response #2912

gerlowskija commented Dec 17, 2024 •

edited

Loading

gerlowskija commented Dec 19, 2024 •

edited

Loading

gerlowskija Dec 23, 2024 •

edited

Loading