SOLR-15745: Move public v2 "core-admin" APIs to annotation framework #565

gerlowskija · 2022-01-25T01:58:06Z

This commit converts the 'reload', 'swap', 'rename', 'unload',
'merge-indexes', and 'split' commands of /v2/cores/<core> over to the
preferred framework.

(NOTE: the spelling 'indexes' in the 'merge-indexes' command was
retained here for consistency/compatibility. We should consider
changing it going forward.)

Description

Solr's been in the slow process of moving its v2 APIs away from the
existing apispec/mapping framework towards one that relies on more
explicit annotations to specify API properties. Many "core-admin"
APIs remain unconverted.

Solution

This commit converts the 'reload', 'swap', 'rename', 'unload',
'merge-indexes', and 'split' commands of /v2/cores/<core> over to the
preferred annotation framework.

Tests

See V2CoreAPIMappingTest

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 main branch.
I have run ./gradlew check.
I have added tests for my changes.

Solr's been in the slow process of moving its v2 APIs away from the existing apispec/mapping framework towards one that relies on more explicit annotations to specify API properties. This commit converts the 'reload', 'swap', 'rename', 'unload', 'merge-indexes', and 'split' commands of `/v2/cores/<core>` over to the preferred framework. (NOTE: the spelling 'indexes' in the 'merge-indexes' command was retained here for consistency/compatibility. We should consider changing it going forward.)

epugh · 2022-01-25T14:10:51Z

Since "v2" is experimental, why not fix "indexes" spelling now?

gerlowskija · 2022-01-25T15:08:47Z

why not fix "indexes" spelling now?

Two "soft" reasons:

It's hard to imagine anyone caring about "indexes" in particular, but in theory I could imagine someone being unhappy at me "burying" API changes in these PRs since I typically describe them as straightforward conversions/translations.
Scope creep - there's so much I'd like to see improved in these APIs that if I started going down those rabbit holes now, I'd never have a hope of finishing the conversion to the annotation framework. (e.g. Why not make SplitCorePayload.ranges a List<String>? Why not unify the use of 'index' and 'core' in some of these commands and property names? Why not add enum support to the annotation framework so we can use it on SplitCorePayload.splitMethod etc.)

That said, I'd buy the argument that "indexes" is a clear spelling error, and I'd be willing to make an exception for it if you think I should @epugh ?

epugh

In general LGTM. Couple of nit picks. You are grinding this out!

epugh · 2022-01-25T14:14:40Z

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

-import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET;
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.PUT;
+import static org.apache.solr.client.solrj.SolrRequest.METHOD.*;


We need to figure out how to make our IDE's not do this!

Agreed, sigh. I play with these IDE settings every now and then when it comes up in review, but I still see it revert from time to time. Not sure what the deal is - I'll try playing with it a bit more.

epugh · 2022-01-25T14:14:52Z

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

-import static org.apache.solr.security.PermissionNameProvider.Name.COLL_READ_PERM;
-import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_EDIT_PERM;
-import static org.apache.solr.security.PermissionNameProvider.Name.CONFIG_READ_PERM;
+import static org.apache.solr.security.PermissionNameProvider.Name.*;


epugh · 2022-01-25T14:22:14Z

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

+ *
+ * @see org.apache.solr.client.solrj.request.beans.MergeIndexesPayload
+ */
+@EndPoint(


I checked CreateCoreAPI, and this looks consistent with how that is annoated ;-)

Cool! (That's a good thing right? I think they should have the same annotations, except that the path on CreateCore should be /cores?)

epugh · 2022-01-25T15:34:41Z

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

+
+import java.util.List;
+
+// TODO rename all of the MergeIndexes* classes to MergeIndices.  These are only named this way for consistency.


Why not do it now? Seperate PR for naming only changes?

epugh · 2022-01-25T15:36:22Z

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

+import java.util.List;
+
+public class SplitCorePayload implements ReflectMapWriter {
+    // Should be converted as an array


is this a bug that we need to fix elsewhere, not in the bounds of this PR, or is this more of a wish? Is there a SOLR- that could be referenced?

Oops - no, this isn't a bug. This was just a "note to self" so I could remember some niggling detail while I was working on this one. I forgot to remove it is all. Will remove.

epugh · 2022-01-25T15:37:28Z

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

+import org.apache.solr.common.util.ReflectMapWriter;
+
+public class SwapCoresPayload implements ReflectMapWriter {
+    @JsonProperty(required = true)


Should we be doing required = true is other places as part of this conversion?

Seems like having some validation on these would be a big win if it's built in to this style of API?

Yep, this enforcement is built in to the annotation-style APIs.

Should we be doing required = true is other places as part of this conversion?

For sure - if there's a place where I skipped required = true that's actually required, absolutely lmk. I thought I set it everywhere I should have, but it's easy to miss.

epugh · 2022-01-25T16:00:02Z

why not fix "indexes" spelling now?

Two "soft" reasons:

1. It's hard to imagine anyone caring about "indexes" in particular, but in theory I could imagine someone being unhappy at me "burying" API changes in these PRs since I typically describe them as straightforward conversions/translations.

2. Scope creep - there's so much I'd like to see improved in these APIs that if I started going down those rabbit holes now, I'd never have a hope of finishing the conversion to the annotation framework.  (e.g. Why not make `SplitCorePayload.ranges` a `List<String>`?  Why not unify the use of 'index' and 'core' in some of these commands and property names? Why not add enum support to the annotation framework so we can use it on `SplitCorePayload.splitMethod` etc.)

That said, I'd buy the argument that "indexes" is a clear spelling error, and I'd be willing to make an exception for it if you think I should @epugh ?

There are 430 results for "Indexes", and honestly, the more I think on it, I think that while "Indices" may be grammatically correct, "Indexes" is really the word we use! At any rate, only 51 "indices" in the source ;-)

epugh · 2022-01-25T16:07:01Z

I thought I was "closing" a comment thread, oops.

madrob · 2022-01-25T16:16:13Z

Would it make sense to move the Payload classes into the API classes? I think that's the only place they are used?

gerlowskija · 2022-01-25T18:53:49Z

There are 430 results for "Indexes", and honestly, the more I think on it, I think that while "Indices" may be grammatically correct, "Indexes" is really the word we use! At any rate, only 51 "indices" in the source

Well, that's another good reason to leave the spelling as-is for now. Yikes though haha 😬

Would it make sense to move the Payload classes into the API classes? I think that's the only place they are used?

The only reason I haven't done that yet is that I have some far-off aspiration of re-using the payload classes in SolrJ as well. It'd be really nice if each API enumerated the params it accepts in only 1 place that can be used on the server and client side alike. (Bit more detail on this idea here)

If you feel strongly about it, I'm open to moving them inside the API classes - I don't want to design too much for a "someday" that may never come. Just lmk. (Either way it should probably be a separate PR since there are a bazillion Payload classes and I'm only touching a few of them here.)

madrob · 2022-01-25T21:42:08Z

Put the new ones inside, leave the old ones?

gerlowskija · 2022-01-26T21:21:20Z

Put the new ones inside, leave the old ones?

It wouldn't be my choice in a vacuum: I can live with giving up on my long-term goal of reusing these payload classes, but introducing the inconsistency for different payloads just seems like kicking me while I'm down 😛

But I guess the inconsistency is only a problem if I'm slow getting a PR together to move the other Payload files. Let's go with this approach. Feel free to bug me in a few days if I don't ping you on a PR to move the rest of the payloads @madrob

gerlowskija · 2022-01-26T21:41:03Z

I think that's everyone's review comments, except for Eric's (totally justified) finger-wagging about my incorrect import settings, which I'm still wrestling with.

I'm going to run gradlew check again and commit shortly. Thanks for the review guys!

…565) Solr's been in the slow process of moving its v2 APIs away from the existing apispec/mapping framework towards one that relies on more explicit annotations to specify API properties. This commit converts the 'reload', 'swap', 'rename', 'unload', 'merge-indexes', and 'split' commands of `/v2/cores/<core>` over to the preferred framework. (NOTE: the spelling 'indexes' in the 'merge-indexes' command was retained here for consistency/compatibility. We should consider changing it going forward.)

Based on review feedback from Mike Drob on PR apache#565

epugh approved these changes Jan 25, 2022

View reviewed changes

epugh closed this Jan 25, 2022

epugh reopened this Jan 25, 2022

SOLR-15745: Address review comments, rd 1

6918e16

gerlowskija merged commit df88d65 into apache:main Jan 27, 2022

gerlowskija deleted the SOLR-15745-annotation-based-v2-public-core-admin-apis branch January 27, 2022 11:41

gerlowskija added a commit to gerlowskija/solr that referenced this pull request Jan 27, 2022

Move all '*Payload' classes inside their API counterparts

e0a1717

Based on review feedback from Mike Drob on PR apache#565

gerlowskija mentioned this pull request Jan 27, 2022

Move all '*Payload' classes inside their API counterparts #570

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15745: Move public v2 "core-admin" APIs to annotation framework #565

SOLR-15745: Move public v2 "core-admin" APIs to annotation framework #565

gerlowskija commented Jan 25, 2022

epugh commented Jan 25, 2022

gerlowskija commented Jan 25, 2022

epugh left a comment

epugh Jan 25, 2022

gerlowskija Jan 25, 2022

epugh Jan 25, 2022

epugh Jan 25, 2022

gerlowskija Jan 25, 2022

epugh Jan 25, 2022

epugh Jan 25, 2022

gerlowskija Jan 25, 2022

epugh Jan 25, 2022

epugh Jan 25, 2022

gerlowskija Jan 25, 2022 •

edited

epugh commented Jan 25, 2022

epugh commented Jan 25, 2022

madrob commented Jan 25, 2022

gerlowskija commented Jan 25, 2022

madrob commented Jan 25, 2022 •

edited

gerlowskija commented Jan 26, 2022

gerlowskija commented Jan 26, 2022


		import java.util.List;

		// TODO rename all of the MergeIndexes* classes to MergeIndices. These are only named this way for consistency.

SOLR-15745: Move public v2 "core-admin" APIs to annotation framework #565

SOLR-15745: Move public v2 "core-admin" APIs to annotation framework #565

Conversation

gerlowskija commented Jan 25, 2022

Description

Solution

Tests

Checklist

epugh commented Jan 25, 2022

gerlowskija commented Jan 25, 2022

epugh left a 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

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 Jan 25, 2022 • edited

Choose a reason for hiding this comment

epugh commented Jan 25, 2022

epugh commented Jan 25, 2022

madrob commented Jan 25, 2022

gerlowskija commented Jan 25, 2022

madrob commented Jan 25, 2022 • edited

gerlowskija commented Jan 26, 2022

gerlowskija commented Jan 26, 2022

gerlowskija Jan 25, 2022 •

edited

madrob commented Jan 25, 2022 •

edited