SOLR-15118: Convert /v2/collections APIs to annotations #2281

gerlowskija · 2021-02-01T14:22:34Z

Description

Solr supports two different ways to write v2 APIs: a JSON spec based
approach, and one based on annotated POJOs. The POJO method is now
preferred.

This commit switches the /v2/collections APIs over to the
annotation-based approach.

Solution

This commit introduces a relatively rote migration of the JSON spec over to annotated POJOs. There's little of note here, except for cases where the v2 JSON spec was missing support for params supported by the v1 APIs. Here I had to make a few decisions as to whether property names should be retained, etc. A good example of this is the v2 backup API, which had several params not covered by the existing v2 spec.

Tests

Adds a new test: V2CollectionsAPIMappingTest.java to test the v2->v1 conversion.

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)
I have developed this patch against the master branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Ref Guide (for Solr changes only).

Solr supports two different ways to write v2 APIs: a JSON spec based approach, and one based on annotated POJOs. The POJO method is now preferred. This commit switches the /v2/collections APIs over to the annotation-based approach.

sigram · 2021-02-04T11:00:08Z

solr/core/src/java/org/apache/solr/handler/CollectionsAPI.java

+                return;
+            }
+
+            toFlatten.forEach((k, v) -> destination.put(additionalPrefix + k, v));


Maybe we should make this recursive to support potentially nested v2 payloads?

I actually tried this initially but found the result to be a lot like the code in CollectionsApiMapping.Meta, which I personally find tough to read with all the map-literals for renaming properties and prefixing properties. I guessed it was more readable to just call this non-recursive version twice. That wouldn't work as well on APIs with deeper nesting, but seemed feasible here.

I'm open to going back that direction though if you prefer it. Did you have a particular signature in mind?

sigram · 2021-02-04T11:01:55Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/beans/BackupCollectionBody.java

+ *
+ * Analogous to the request parameters for v1 /admin/collections?action=BACKUP API.
+ */
+public class BackupCollectionBody implements ReflectMapWriter {


What do you think about renaming all of these to use *Payload instead of *Body?

sigram · 2021-02-04T11:08:00Z

solr/solrj/src/java/org/apache/solr/client/solrj/request/beans/CreateBody.java

+    @JsonProperty
+    public Boolean perReplicaState;
+
+    public static class RouterInfo implements ReflectMapWriter {


Router spec may in fact contain arbitrary properties in addition to these two - see DocRouter.getRouterSpec. Currently only name and field are used in the routers that Solr provides but other users may use some other properties in their implementations.

Good catch. There's a Jackson annotation made for this case (JsonAnyGetter) - named properties will be mapped as usual but "extra" ones are put in the catchall Map. But it looks like we're not actually using Jackson here to avoid SolrJ needing to depend on it. The JsonProperty annotation is a copy in our own code to avoid the dep.

I could create clone JsonAnyGetter the way we've done with JsonProperty, but idt it's worth it. I'll just remove this RouterInfo class and handle all the properties as a Map for the translation.

Might be worth considering whether a Jackson-dep is worth it after all going forward though.

noblepaul · 2021-02-04T11:19:20Z

Thanks for picking this up @gerlowskija

Please let me know if you need any help

sigram · 2021-02-04T11:19:43Z

solr/core/src/java/org/apache/solr/handler/CollectionsAPI.java

+    public static final String V2_RESTORE_CMD = "restore-collection";
+    public static final String V2_CREATE_ALIAS_CMD = "create-alias";
+    public static final String V2_SET_ALIAS_PROP_CMD = "set-alias-property";
+    public static final String V2_DELETE_ALIAS_CMD = "delete-alias";


Should we also add other collection commands here, such as: COLLECTIONPROP, CREATE / DELETESHARD, ADD / MOVE / DELETEREPLICA, MODIFYCOLLECTION, REINDEXCOLLECTION, RENAME, SPLITSHARD ?

Yes, please

They should definitely exist somewhere, but they're not /v2/collections APIs, so I didn't do that in this PR. If you guys really want them, I'm not against adding those constants in this PR.

But even then they should probably live somewhere else, shouldn't they? (since this class is for /v2/collections APIs, and those commands are for other APIs? AFAIK this file is only for those APIs that specifically live under /v2/collections, right?)

gerlowskija · 2021-02-04T18:28:20Z

Hey, thanks for offering @noblepaul . I do have one thing I was puzzling over:

How is SolrJ's getV2Request/V2RequestSupport feature intended to work for annotation-based v2 APIs? It looks like V1ToV2ApiMapper is really dependent on CollectionApiMapping.Meta, which the annotation-based APIs can't be found in obviously.

There are some annotation-based v2 APIs whose SolrJ request classes implement V2RequestSupport (e.g. CollectionAdminRequest.ClusterProp), so there should be logic somewhere to handle that I imagine? Unless getV2Request will blow up now if called on certain CollectionAdminRequest objects?

More broadly I wonder whether there's any value in getV2Request going forward. It never really "took off" to cover APIs other than CollectionAdmin, and there might be better ways to let SolrJ users dip their toes into the v2 waters (e.g. dedicated request classes that better align with the v2 paths/concepts? idk).

If we don't have an approach for V2RequestSupport + annotation-based APIs worked out, and there's not strong feeling about carrying V2RequestSupport forward, I'd lean towards making this PR a master-only change, and deprecating getV2Request in 8.9 so it can be removed by this PR on master.

The V2RequestSupport interface is incompatible with annotated-POJO-based v2 APIs. (At least, without a good bit of code duplication.) Since some of CollectionAdmin commands covered by CollectionAdminRequest now have annotated-POJO-based v2 equivalents, CAR is better off not implementing V2RequestSupport.

gerlowskija · 2021-02-08T15:09:08Z

Going to merge this with the strategy for dealing with V2RequestSupport that I mentioned in my last comment.

If you eventually get a chance to reply to that comment above @noblepaul , lmk if you prefer some other approach and I can pick it back up then.

Solr supports two different ways to write v2 APIs: a JSON spec based approach, and one based on annotated POJOs. The POJO method is now preferred. This commit switches the /v2/collections APIs over to the annotation-based approach. Since V2RequestSupport only works with jsonspec-based APIs, this commit also changes CollectionAdminRequest to no longer implement that interface.

gerlowskija added 3 commits February 1, 2021 09:16

Use constants for param names where possible

c613a1f

Fix precommit, rd 1

472b7a4

sigram reviewed Feb 4, 2021

View reviewed changes

gerlowskija added 6 commits February 4, 2021 08:58

Merge branch 'master' into SOLR_15118_annotation_based_collections_apis

f62f5b8

Add new incremental backup/restore params to v2 POJOs

7b06110

Remove unintentionally committed 'IML' file

e4a0997

Address review comments, rd 1

f68afd1

Address review feedback, rd 1 (cont'd)

253676f

Fix broken tests, pt 1

6c616ad

gerlowskija added 4 commits February 6, 2021 21:54

Fix precommit, rd 1

5ffe72d

Fix precommit, rd 2

2f025b7

Fix test failures, rd 1

7ec7382

gerlowskija merged commit e89fba6 into apache:master Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15118: Convert /v2/collections APIs to annotations #2281

SOLR-15118: Convert /v2/collections APIs to annotations #2281

gerlowskija commented Feb 1, 2021

sigram Feb 4, 2021

gerlowskija Feb 4, 2021

sigram Feb 4, 2021

sigram Feb 4, 2021

gerlowskija Feb 4, 2021 •

edited

noblepaul commented Feb 4, 2021

sigram Feb 4, 2021

noblepaul Feb 4, 2021

gerlowskija Feb 4, 2021

gerlowskija commented Feb 4, 2021 •

edited

gerlowskija commented Feb 8, 2021

SOLR-15118: Convert /v2/collections APIs to annotations #2281

SOLR-15118: Convert /v2/collections APIs to annotations #2281

Conversation

gerlowskija commented Feb 1, 2021

Description

Solution

Tests

Checklist

sigram Feb 4, 2021

Choose a reason for hiding this comment

gerlowskija Feb 4, 2021

Choose a reason for hiding this comment

sigram Feb 4, 2021

Choose a reason for hiding this comment

sigram Feb 4, 2021

Choose a reason for hiding this comment

gerlowskija Feb 4, 2021 • edited

Choose a reason for hiding this comment

noblepaul commented Feb 4, 2021

sigram Feb 4, 2021

Choose a reason for hiding this comment

noblepaul Feb 4, 2021

Choose a reason for hiding this comment

gerlowskija Feb 4, 2021

Choose a reason for hiding this comment

gerlowskija commented Feb 4, 2021 • edited

gerlowskija commented Feb 8, 2021

gerlowskija Feb 4, 2021 •

edited

gerlowskija commented Feb 4, 2021 •

edited