gerlowskija commented on code in PR #1080:
URL: https://github.com/apache/solr/pull/1080#discussion_r997139202
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameClusterAPI.java:
##########
@@ -0,0 +1,46 @@
+package org.apache.solr.handler.admin.api;
+
+import org.apache.solr.api.Command;
+import org.apache.solr.api.EndPoint;
+import org.apache.solr.api.PayloadObj;
+import org.apache.solr.client.solrj.request.beans.RenameClusterPayload;
+import org.apache.solr.common.params.CollectionAdminParams;
+import org.apache.solr.common.params.CollectionParams;
+import org.apache.solr.handler.admin.CollectionsHandler;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+
+import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
+import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static
org.apache.solr.security.PermissionNameProvider.Name.COLL_EDIT_PERM;
+
+@EndPoint(
+ path = {"/clusters/rename/{cluster}"},
+ method = POST,
+ permission = COLL_EDIT_PERM)
+public class RenameClusterAPI {
Review Comment:
[-1] The RENAME operation (afaik?) renames a "collection", not a "cluster".
(Actually, I don't even think a SolrCloud cluster has a name, in that sense...)
So we should probably change the name of a few things here to make it
clearer what is actually being renamed. `RenameCollectionAPI` maybe?
The API should probably also change, since that path implies that it's the
cluster being renamed, not an individual collection.
I put together a
[spreadsheet](https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA/edit?usp=sharing)
awhile back laying out Solr's current APIs (with some suggestions on how to
close gaps and be a bit more consistent). It's not authoritative strictly
speaking, so we can definitely consider other options, but that sheet proposes
the following for our v2 rename operation: `POST
/api/collections/origCollName/commands/rename {"to": "newName"}`
What do you think?
[0] I suspect you just hadn't gotten to it yet, but don't forget to go into
CollectionsHandler.getApis() and add a line there to register this new API!
##########
solr/core/src/java/org/apache/solr/handler/admin/api/RenameClusterAPI.java:
##########
@@ -0,0 +1,46 @@
+package org.apache.solr.handler.admin.api;
Review Comment:
[-1] Source code in Solr uses a particular copyright header in all files,
and we'll need to add this here. (You can find it at the very top of any
`.java` source file and copy/paste it from there).
I have a really hard time remembering this, so I've found it really helpful
to have my editor add this into all new files automatically by setting up
what's essentially a "template" for new source code files. I use IntelliJ
personally, so I'll link the docs for that one
[here](https://www.jetbrains.com/help/idea/using-file-and-code-templates.html#create-new-template).
##########
solr/core/src/test/org/apache/solr/handler/admin/api/V2ClusterAPIMappingTest.java:
##########
@@ -0,0 +1,50 @@
+package org.apache.solr.handler.admin.api;
+
+import org.apache.solr.api.ApiBag;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.handler.ClusterAPI;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.handler.admin.ConfigSetsHandler;
+import org.apache.solr.handler.admin.V2ApiMappingTest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
+
+import static org.apache.solr.common.params.CollectionAdminParams.COLLECTION;
+import static org.apache.solr.common.params.CollectionAdminParams.TARGET;
+import static org.apache.solr.common.params.CommonParams.ACTION;
+import static org.mockito.Mockito.mock;
+
+
+public class V2ClusterAPIMappingTest extends
V2ApiMappingTest<CollectionsHandler> {
+
+@Test
+public void testRenameClusterAllParams() throws Exception {
Review Comment:
[-1] Is this the test that failed with the error message you mentioned in
JIRA?
```
org.apache.solr.handler.admin.api.V2CollectionAPIMappingTest >
testRenameCluster FAILED
java.lang.NullPointerException: Cannot invoke
"org.apache.solr.api.Api.call(org.apache.solr.request.SolrQueryRequest,
org.apache.solr.response.SolrQueryResponse)" because "api" is null
```
Or has this test changed since you posted that message? Well, obviously you
moved the test-case into its own class I guess, but have there been other
changes? Are you still seeing the test fail in the same way?
Assuming you are...
One thing that jumps out to me is that `populateApiBag` is unimplemented
here, which would probably cause the failure you mentioned in JIRA.
Some helpful context, `ApiBag` is how Solr traditionally matches v2 API
requests to specific API classes/methods. On startup, Solr loads all the APIs
into this "bag" class, and then at request time it [looks
up](https://github.com/apache/solr/blob/6786ec032727d7d21cfdfb5705d60f4d8368a1cd/solr/core/src/java/org/apache/solr/api/ApiBag.java#L340)
the right API based on the HTTP path, method, etc.
This test doesn't run a full Solr though, so for it to find and run your
API, you need to write a bit of code to add it yourself in the `populateApiBag`
method. Check out the `populateApiBag` and `createUnderlyingRequestHandler`
implementations on [one of the other "Mapping"
tests](https://github.com/apache/solr/blob/db23d7d48d5ef2cf84ff8f8de38209698db172d4/solr/core/src/test/org/apache/solr/handler/admin/V2CollectionsAPIMappingTest.java#L54)
for examples on how this works.
Hope that helps!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
