SOLR-16490 Create a v2 equivalent for RESTORECORE#1449
SOLR-16490 Create a v2 equivalent for RESTORECORE#1449gerlowskija merged 5 commits intoapache:mainfrom
Conversation
|
@gerlowskija @chatman Request you to advise on the next steps |
|
Hi @sayandigital - sorry for the delay, this got buried in my Inbox for awhile. But thanks for picking this up; will aim to review shortly! |
Thank you @gerlowskija |
gerlowskija
left a comment
gerlowskija
left a comment
There was a problem hiding this comment.
Thanks for the PR @sayandigital! Overall the PR looks good; just need to tweak the appearance of the v2 API a bit and make sure we're supporting all the parameters that the v1 RESTORECORE API supports.
Let me know if my inline comments are unclear or there's anything else I can help with!
| * @see RestoreCorePayload | ||
| */ | ||
| @EndPoint( | ||
| path = {"/cores/{core}"}, |
There was a problem hiding this comment.
[-1] IMO the endpoint specifics (particularly the path/url) should look a little different here.
It might've gotten buried in all the text on SOLR-16490, but we've been building up a spreadsheet of Solr's APIs and what their v2 equivalents will ideally look like. RESTORECORE is a bit hard to find there, but on the "'Command' APIs" Tab there's an entry for it. The path there is listed as /cores/{core}/restore- could we use that value here?
| this.coreAdminHandler = coreAdminHandler; | ||
| } | ||
|
|
||
| @Command(name = V2_CORE_RESTORE_CMD) |
There was a problem hiding this comment.
[-1] Similar my comment above about the API path, I think the overall shape of this code looks great. Just need to tweak a few specifics so that the v2 RESTORECORE API lines up better with the direction we're trying to move our v2 APIs in.
Particularly, we're trying to move away from the v2 "command-based" APIs (i.e. APIs with a request body whose only top-level key is the name of some command or other). Instead we're generally moving the command itself into the path (see my comment above about changing the path to /cores/coreName/restore) and then moving most of the actual restore parameters to the top level of the "Payload"/request-body.
This is definitely do-able using the Solr-custom annotation framework you've used in this PR (see RenameCollectionAPI for an example). But you might find it more natural to do if you switch this API over to use the JAX-RS framework we've been moving towards. If you want to try that out there's a step-by-step writeup on this here and a handful of examples that you can crib from (example1, example2)
|
|
||
| import org.apache.solr.common.util.ReflectMapWriter; | ||
|
|
||
| public class RestoreCorePayload implements ReflectMapWriter {} |
There was a problem hiding this comment.
[0] RestoreCore operations supports a bunch of params that we'll need to include in the payload/request-body here. For a sense of what all 'restore' supports, see the req.getParams() calls in RestoreCoreOp
|
Hi @sayandigital - thanks again for your work on this. Let me know if I can help out with any questions or issues that crop up when you get a chance to circle back to this! |
Also addresses a number of review comments and adds some small bits of test coverage.
|
Alright - I've made some changes on top of the base you provided @sayandigital. Both to address some of the review comments, and to use the more-preferred JAX-RS framework that Solr supports now. Tests and 'check' pass; gonna merge this afternoon and then let things bake for a few days before backporting. |
No v2 equivalent existed prior to this commit. The new V2 API is
`POST /api/cores/cName/restore {...}`.
---------
Co-authored-by: Jason Gerlowski <gerlowskija@apache.org>
|
Thanks @gerlowskija to help take this to completion |
2 participants
https://issues.apache.org/jira/browse/SOLR-16490
Description
Create a v2 equivalent for /admin/cores for RESTORECORE
Solution
Adding a new class called RestoreCoreAPI and RestoreCorePayload as per the V2 API documentation provided in the document( Solr v2 API proposed changes https://docs.google.com/spreadsheets/d/1HAoBBFPpSiT8mJmgNZKkZAPwfCfPvlc08m5jz3fQBpA/edit#gid=1549066254)
Tests
Added test cases in solr/core/src/test/org/apache/solr/handler/admin/api/V2CoreAPIMappingTest.java
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.