SOLR-15749: Create v2 equivalent for 'rename' collection API #1080

AAnakhe · 2022-10-15T19:02:44Z

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

Description

Create a v2 equivalent of v1 'RENAME' /solr/admin/collections endpoint

Solution

Created a RenameClusterAPI class and RenameClusterPayload class with an endpoint @endpoint(path ={"/clusters/rename/{cluster}"}, method = POST, permission = COLL_EDIT_PERM)

Tests

Tested mapping to API endpoint

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.
[] I have added documentation for the Reference Guide

This reverts commit fb50537.

sonatype-lift · 2022-10-15T19:51:32Z

⚠️ 314 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

gerlowskija

Thanks for the PR @AAnakhe !

I left a few comments on things that'll need fixed, along with hopefully some help for the test issue you mentioned in JIRA! Lmk what you think when you get a chance to read through!

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

solr/core/src/test/org/apache/solr/handler/admin/api/V2ClusterAPIMappingTest.java

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

• Renamed, RenameClusterAPI class to RenameCollectionAPI and corresponding implementations • Renamed, RenameClusterPayload class to RenameCollectionPayload • Removed V2ClusterAPIMappingTest class and implemented test method in existing V2CollectionAPIMappingTest • Added apache copyright header to all classes Result: Test passed

gerlowskija

Hey Anakhe - added a few more comments (or follow-ups to comments from my previous review). Let me know if there's any that you need more clarification on, etc.

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

solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java

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

AAnakhe · 2022-10-31T14:28:27Z

Okay thanks, I will fix that shortly

…

On Mon, Oct 31, 2022 at 3:21 PM Jason Gerlowski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java <#1080 (comment)>: > @@ -85,6 +85,18 @@ public void testGetCollectionStatus() throws Exception { assertEquals("shard2", v1Params.get(SHARD)); } + + @test + public void testRenameCollectionAllParams() throws Exception { + final SolrParams v1Params = captureConvertedV1Params( + "/collections/collName", "POST", "{\"rename\": {\"to\": \"targetColl\"}}"); The first thing that jumps out at me in this test code is that you restructured the API endpoint to look the way we discussed. (i.e. now it looks like POST /collections/collName/commands/rename)....but this test still uses the old appearance/syntax. The path, request-body, etc. that gets sent into captureConvertedV1Params is all using the old syntax we moved away from. If you fix that, I'm betting this error will go away! As a general note on "process": generally speaking its easier for me (or others) to help review if you push up the code changes you've made, even if things aren't 100% working yet. That makes it easier for me to comment on particular lines. It makes it easier for me to pull the code down and run that test myself, etc. It saves you from having to paste your code in a comment box and fight the markdown/formatting, etc. Totally fine if you had reasons for not doing that here; just figured I'd mention the convention as a "heads up" in case it's helpful since you're newer to the community — Reply to this email directly, view it on GitHub <#1080 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWIEDZG2KINRYWJVZZDM74TWF7IYLANCNFSM6AAAAAARGAWM5E> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Test result returns java.lang.AssertionError: expected:<rename> but was:<null>

joshgog · 2022-11-01T06:37:12Z

solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java

-    final SolrParams v1Params = captureConvertedV1Params(
-            "/collections/collName", "POST", "{\"rename\": {\"to\": \"targetColl\"}}");
+  public void testRenameCollectionAllParams(){
+    final SolrQueryRequest request = runRenameCollectionsApi("/collections/collName");


Test result returns java.lang.AssertionError: expected:<rename> but was:<null>

What if we mock SolrQueryRequest and SolrQueryResponse , pass them to thepublic void renameCollection(SolrQueryRequest req, SolrQueryResponse rsp) method of the API then perform the assertions

I will try to implement that, thanks

Test result returns java.lang.AssertionError: expected:<rename> but was:<null>

What if we mock SolrQueryRequest and SolrQueryResponse , pass them to thepublic void renameCollection(SolrQueryRequest req, SolrQueryResponse rsp) method of the API then perform the assertions

Please I need more clarification on this

I'm not sure what exactly you would like to be clarified. If its about mocks checkout AddReplicaPropertyAPITest which uses mocked SolrQueryRequest and SolrQueryResponse

+1 to what Josh said. AddReplicaPropertyAPITest is a good example on how to use mocks, but it's setup for APIs that use the JAX-RS framework, so you wouldn't want to follow it too closely. But it's good if you're just looking for an example on the mock stuff.

And I'm sure we'd both be happy to clarify if you can elaborate a bit on what you need clarified.

[Q] One thing that jumps out at me on this line is the runRenameCollectionsAPI: is there a reason you've created a separate method for this instead of reusing one of the captureConvertedV1Params methods that's already out there. If there's not some reason for runRenameCollectionsAPI that I'm missing, you could presumably just replace its usage here with a snippet like:

final SolrParams v1Params = captureConvertedV1Params("/collections/collName/commands/rename", "POST", "{\"to\": \"targetColl\", \"async\": \"requestTrackingId\", \"followAliases\": true}");

The second thing that jumps out at me here is the path being sent as an argument to this method - it doesn't match the path of the API you created in this PR (see here).

I thought you said we were moving away from the old captureConvertedV1Params syntax, I wasn't aware there are different method implementations

Ah, not quite. Or at least, I didn't mean to suggest that?

What I probably meant to say is that Solr is moving away from its traditional v2 API framework to a framework based on JAX-RS. captureConvertedV1Params is a nice utility for testing APIs implemented using the "traditional" (i.e. non JAX-RS) framework.

In the long run I'd expect to see usage of captureConvertedV1Params decline, but not because it's deprecated or shouldn't be used - just because things will switch over to JAX-RS as time goes by. This PR uses the traditional framework (which is absolutely still valid), so it's fine/normal to use captureConvertedV1Params in your tests.

I'll push up a commit here to help get the unit test over the line and then hopefully we can get this merged 👍

It's clearer now, and I'm so glad to hear that this will be merged thank you.

gerlowskija

Going through some of my reviews this afternoon just to summarize a bit where things stand:

There are a few small bugs in this PR that I've added line-level comments for: mostly small stuff. Other than those, the only outstanding issue is getting that V2CollectionAPIMappingTest unit test passing. I'll give you a little more time to respond to Josh's and my suggestions there (see the line-level comments on that file), but I'll try to take a look at the test myself next week to see if there's any obvious issue we can help you through.

Enjoy your weekend!

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

gerlowskija · 2022-11-04T19:40:11Z

solr/core/src/test/org/apache/solr/handler/admin/api/V2CollectionAPIMappingTest.java

-    final SolrParams v1Params = captureConvertedV1Params(
-            "/collections/collName", "POST", "{\"rename\": {\"to\": \"targetColl\"}}");
+  public void testRenameCollectionAllParams(){
+    final SolrQueryRequest request = runRenameCollectionsApi("/collections/collName");


+1 to what Josh said. AddReplicaPropertyAPITest is a good example on how to use mocks, but it's setup for APIs that use the JAX-RS framework, so you wouldn't want to follow it too closely. But it's good if you're just looking for an example on the mock stuff.

And I'm sure we'd both be happy to clarify if you can elaborate a bit on what you need clarified.

[Q] One thing that jumps out at me on this line is the runRenameCollectionsAPI: is there a reason you've created a separate method for this instead of reusing one of the captureConvertedV1Params methods that's already out there. If there's not some reason for runRenameCollectionsAPI that I'm missing, you could presumably just replace its usage here with a snippet like:

final SolrParams v1Params = captureConvertedV1Params("/collections/collName/commands/rename", "POST", "{\"to\": \"targetColl\", \"async\": \"requestTrackingId\", \"followAliases\": true}");

The second thing that jumps out at me here is the path being sent as an argument to this method - it doesn't match the path of the API you created in this PR (see here).

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

AAnakhe · 2022-11-04T21:11:54Z

Thank you Jason, you do the same.

…

On Sat, Oct 15, 2022 at 8:51 PM sonatype-lift[bot] ***@***.***> wrote: ⚠️ *314 God Classes* were detected by Lift in this project. Visit the Lift web console <https://lift.sonatype.com/results/github.com/apache/solr/01GFEGDZ81KM2ZP1W0ZBMPP3N9?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache%20solr> for more details. — Reply to this email directly, view it on GitHub <#1080 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWIEDZD65G22DAN66STNGC3WDMDM5ANCNFSM6AAAAAARGAWM5E> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Test failed: java.lang.NullPointerException: Cannot invoke "java.lang.Iterable.iterator()" because the return value of "org.apache.solr.request.SolrQueryRequest.getContentStreams()" is null

Includes fix for V2CollectionAPIMappingTest

This at least adds a corresponding v2 snippet. Note that this section of the ref-guide is missing several parameters that the API does support. Ideally we'd backfill these, but I've held off here out of healthy respect for scope creep.

gerlowskija

Alright, I've added a few commits to this PR to help move things along.

Most of it is cleanup, with the exception of a small test fix in V2CollectionAPIMappingTest, and some ref-guide coverage for the new v2 API. Take a look at the new commits if you're curious.

I think this should be ready for merge now, once the tests and 'check' pass. If anyone else sees anything I'm missing, lmk!

gerlowskija · 2022-11-08T20:07:57Z

build.gradle

@@ -207,5 +207,5 @@ apply from: file('gradle/ant-compat/solr.post-jar.gradle')

 apply from: file('gradle/node.gradle')

-sourceCompatibility = JavaVersion.VERSION_17
-targetCompatibility = JavaVersion.VERSION_17
+//sourceCompatibility = JavaVersion.VERSION_16_PREVIEW


[Q] Ooh, interesting. Did you intend to edit these lines @AAnakhe ?

A few months back I inadvertently committed changes to these two lines, but to this day I have no memory of editing them. I suspected it came from importing into my IDE or some similar operation but I never was able to figure out what did it.

Anyway, just curious if you intended these changes here. And if you didn't, if you had any guesses what might've edited them on your behalf?

I didn't intend to make any changes there, it was imported by my IDE when I clicked on a confirmation request from IntelliJ requesting support for Java 16 or so, I can not recall correctly

gerlowskija · 2022-11-08T20:16:40Z

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

+            .disable(MapperFeature.AUTO_DETECT_FIELDS);
+
+    @EndPoint(
+            path = {"/collections/{collection}/commands/rename"},


[0] There was some discussion on a different v2 API PR about the /commands path segment in this path, and the consensus was that we should drop that path segment.

I'll update the code here accordingly.

joshgog and others added 4 commits October 11, 2022 00:51

Created v2 equivalent of v1 'CLUSTERSTATUS' command

fb50537

Created a v2 equivalent of v1 'RENAME'

9273f9f

Revert "Created v2 equivalent of v1 'CLUSTERSTATUS' command"

edd217a

This reverts commit fb50537.

Test Class for V2 RenameClusterAPI

ec887df

AAnakhe changed the title ~~SOLR 15749 created a v2 equivalent of v1 'rename' #1061~~ SOLR 15749 created a v2 equivalent of v1 'rename' Oct 15, 2022

gerlowskija requested changes Oct 17, 2022

View reviewed changes

gerlowskija requested changes Oct 20, 2022

View reviewed changes

Update: Text method updated to V2 syntax

9109a59

Test result returns java.lang.AssertionError: expected:<rename> but was:<null>

joshgog reviewed Nov 1, 2022

View reviewed changes

gerlowskija requested changes Nov 4, 2022

View reviewed changes

AAnakhe and others added 4 commits November 8, 2022 03:53

Update: V2RenameCollectionAPItest

6f7c37b

Test failed: java.lang.NullPointerException: Cannot invoke "java.lang.Iterable.iterator()" because the return value of "org.apache.solr.request.SolrQueryRequest.getContentStreams()" is null

Merge branch 'main' into aanakhe_2

f497b74

Minor cleanup and updates

5c86d51

Includes fix for V2CollectionAPIMappingTest

Add coverage for v2 RENAME to ref-guide

975d1e6

This at least adds a corresponding v2 snippet. Note that this section of the ref-guide is missing several parameters that the API does support. Ideally we'd backfill these, but I've held off here out of healthy respect for scope creep.

gerlowskija reviewed Nov 9, 2022

View reviewed changes

gerlowskija changed the title ~~SOLR 15749 created a v2 equivalent of v1 'rename'~~ SOLR-15749: Create v2 equivalent for 'rename' collection API Nov 9, 2022

gerlowskija approved these changes Nov 9, 2022

View reviewed changes

Add CHANGES.txt entry

50e3ac9

gerlowskija merged commit ed93594 into apache:main Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15749: Create v2 equivalent for 'rename' collection API #1080

SOLR-15749: Create v2 equivalent for 'rename' collection API #1080

AAnakhe commented Oct 15, 2022 •

edited

sonatype-lift bot commented Oct 15, 2022

gerlowskija left a comment

gerlowskija left a comment

AAnakhe commented Oct 31, 2022 via email

joshgog Nov 1, 2022 •

edited

AAnakhe Nov 1, 2022

AAnakhe Nov 1, 2022

joshgog Nov 1, 2022 •

edited

gerlowskija Nov 4, 2022

AAnakhe Nov 8, 2022

gerlowskija Nov 8, 2022

AAnakhe Nov 9, 2022

gerlowskija left a comment

gerlowskija Nov 4, 2022

AAnakhe commented Nov 4, 2022 via email

gerlowskija left a comment

gerlowskija Nov 8, 2022

AAnakhe Nov 12, 2022

gerlowskija Nov 8, 2022

SOLR-15749: Create v2 equivalent for 'rename' collection API #1080

SOLR-15749: Create v2 equivalent for 'rename' collection API #1080

Conversation

AAnakhe commented Oct 15, 2022 • edited

Description

Solution

Tests

Checklist

sonatype-lift bot commented Oct 15, 2022

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija left a comment

Choose a reason for hiding this comment

AAnakhe commented Oct 31, 2022 via email

joshgog Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshgog Nov 1, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAnakhe commented Nov 4, 2022 via email

gerlowskija 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

AAnakhe commented Oct 15, 2022 •

edited

joshgog Nov 1, 2022 •

edited

joshgog Nov 1, 2022 •

edited