Repository navigation
SOLR-16398: Migrate rebalance leaders and migrate docs v2 APIs to JAX-RS - #5284
nick-boss-tech wants to merge 8 commits into
Conversation
|
I am thinking of splitting this PR in two, one per command: rebalance leaders on its own, migrate docs on its own. That makes two PRs of about 11 files each instead of one at 18, and each can be reviewed and merged independently. Separately, this PR is still blocked by #5285: the 🤖 AI-generated text above 🤖 |
gerlowskija
left a comment
There was a problem hiding this comment.
Thanks for the changes @nick-boss-tech !
I think there's a little room for improvement though, and left comments to a few specific places "in line". I'm not sure if I've reviewed any of your changes yet, so just a heads up on a convention I use: I annotate comments that I'd like changed but don't feel strongly about with [-0]; things I feel pretty certain should be changed get the stronger [-1].
I think the only -1 comments here are about one of the test classes that I think should be reworked. Other than that, things look great!
| @@ -0,0 +1,7 @@ | |||
| title: Migrate the v2 rebalance-leaders and migrate-docs APIs to JAX-RS | |||
There was a problem hiding this comment.
[-0] These changelog entries ultimately end up in front of our users, and I kindof doubt most users care about "minor" details like what framework is used under-the-hood to implement the APIs.
IMO changelog entries for v2 APIs should highlight either the changes to the API cosmetics, or the SolrJ request/response classes that users can now start using. (Converting an API to JAX-RS has it picked up by our OpenAPI Specification, which we use to generate client code for Java and other languages. So while customers likely don't care about JAX-RS in and of itself...they probably do care that about the new SolrJ goodies that a JAX-RS migration implicitly creates.)
| @Operation( | ||
| summary = "Rebalance shard leaders in a collection according to preferred leader settings.", | ||
| tags = {"collections"}) | ||
| FlexibleSolrJerseyResponse rebalanceLeaders( |
There was a problem hiding this comment.
[-0] FlexibleSolrJerseyResponse technically works, but it would better to create a new response type that provides a richer description of what we expect the response to look like. This response type gets incorporated into our OpenAPI Specification and there's client code generated from that, so the more detail we can give about what users should actually expect this response to look like, the better.
The best way to do this is to read through the rebalance-leaders logic used by both v1 and v2 and figure out what fields it's looking for and setting in the response. For example, you can see that the response needs at least a "summary" field.
| } | ||
| solrQueryRequest.setParams(params); | ||
|
|
||
| new org.apache.solr.handler.admin.RebalanceLeaders( |
There was a problem hiding this comment.
[-0] Wdyt about reworking org.apache.solr.handler.admin.RebalanceLeaders to fit with our v2 approach a little better. It looks like we're the only caller of this class, so rather than it taking in a SolrQueryRequest/SolrQueryResponse (very v1-specific classes), we could modify its to consume the strongly-type parameters we have access to here.
That'd make this code look much nicer as most of the SolrParams-building code here could be dropped.
If we don't want to do it for scope reasons, fair enough, but IMO it's worth considering.
| import org.junit.Test; | ||
|
|
||
| /** Drives the JAX-RS collection command APIs over HTTP against a running SolrCloud cluster. */ | ||
| public class CollectionCommandsV2IntegrationTest extends SolrCloudTestCase { |
There was a problem hiding this comment.
[-1] I love having integration tests like this for the v2 APIs, but IMO they should use the generated SolrJ request/response classes instead of the somewhat "raw" V2Request/V2Response usage. Two reasons for that:
- It gives us validation for the generated code, which otherwise doesn't have test coverage.
- Dogfooding - starting to use these generated client classes will help us notice things that we wish were different about them.
[-1] Do we need a whole new integration test class for this functionality, or could we accomplish the same coverage by adding or modifying existing test classes? TestRebalanceLeaders and MigrateRouteKeyTest already have similar "full SolrCloud" tests for these capabilities. If there's not a specific reason you think we should create a new test class, let's push the contents of this file down into those two test classes that already exist.
🤖 AI text below 🤖 (reviewed by Nick Shanin)
Draft status is due to Nick Shanin's open-PR cap; the change is complete and ready for review, and it will be marked Open upon request.
This is the next sibling PR under SOLR-16398, following the split-shard migration in PR #5265, which Eric Pugh asked for as the representative command before the approach scaled up to the remaining commands.
What happens today
Rebalance leaders and migrate docs are still served by annotated v1-style API classes while the shard APIs around them are JAX-RS resources.
RebalanceLeadersAPIandMigrateDocsAPIcarry their v2 routes as annotations on v1-shaped classes registered throughCollectionsHandler.getApis(). The neighboring collection commands (create shard, delete shard, split shard) are Jersey resources with endpoint interfaces insolr/api, typed request bodies, and generated SolrJ clients. The v2 tracking spreadsheet lists both commands as not yet migrated to JAX-RS.What this change does
Both commands become JAX-RS resources in the same shape as split shard, and the annotated classes are removed.
New endpoint interfaces
RebalanceLeadersApiandMigrateDocsApi, with typed request bodies; the migrate-docs body carries theasyncfield, and the resource passes it to the same submission the v1 route uses. The resourcesRebalanceLeadersandMigrateDocsare registered inCollectionsHandleralongsideSplitShard, andRebalanceLeadersAPIandMigrateDocsAPIare deleted. The v1 routes do not change behavior: MIGRATE still returns its overseer message from the handler, and REBALANCELEADERS still executes the admin operation directly.A choice to check
The two commands share one PR, and both endpoints carry the request-body fix from PR #5265.
The family plan on the ticket, set by Jason Gerlowski, is one to two APIs per PR; these two share their handler registration and test scaffolding, so they travel together. Shout if you would rather see them split. Separately, PR #5265 recorded that the sibling endpoints carried the same bare
@RequestBodydefect fixed there; both endpoint interfaces here declare@RequestBody(description = "Contains user provided parameters"), so the generated client and OpenAPI spec include the request body.Proof
The focused tests pass at the PR head; on the base code the v2 resources and the integration test do not exist.
Verified 2026-11-03 at head
793e1ffa0f1d67271fbf090773651cd8a26171e4, on a branch rebased onto main after PR #5265 merged: tidy clean for theapiandcoremodules;:solr:core:compileJavapasses with Error Prone enabled (-Pvalidation.errorprone=true);CollectionCommandsV2IntegrationTest(2 tests),V2CollectionAPIMappingTest(2 tests),TestCollectionAPIs(2 tests), andTestApiFramework(3 tests) all pass with no failures, errors, or skips;:solr:core:check -x testpasses.Limits
Only rebalance leaders and migrate docs are in this PR.
Move replica follows on its own branch, and the core commands and the reindex migration are likewise ready on branches for later PRs, per the plan on the ticket. Replication is not covered here: Jalaz Kumar is carrying those APIs forward, per the ticket discussion. Neither command needed the v1-to-v2 async adapter that split shard needed, because neither v1 operation delegates to a resource: MIGRATE returns its message to the handler, which submits it, and REBALANCELEADERS executes in place.
Changelog: changelog/unreleased/SOLR-16398-rebalance-migrate.yml
Nick Shanin directed the work and takes responsibility for this contribution.