Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-16398: Tweak v2 B-S-U API to be more REST-ful #1689

Conversation

gerlowskija
Copy link
Contributor

@gerlowskija gerlowskija commented Jun 4, 2023

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

Description

Solr has slowly been modifying its v2 APIs to align around a more REST-ful model. Many APIs have already been through this re-alignment but many more remain, including Solr's "balance-shard-unique" functionality.

Solution

This PR modifies the v2 "balance-shard-unique" API to be more REST-ful: changing the path, and removing the "command-specifier" from the request body. With these changes the functionality can now be invoked as:

POST /api/collections/someCollectionName/balance-shard-unique {"property": "preferredLeader"}

To facilitate this change, this PR also migrates the v2 API definition to the JAX-RS framework.

Tests

New unit tests in BalanceShardUniqueAPITest. Existing tests continue to pass.

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

BalanceShardUniqueAPI.createRemoteMessage("someCollName", requestBody).getProperties();

assertEquals(6, remoteMessage.size());
assertEquals("balanceshardunique", remoteMessage.get(QUEUE_OPERATION));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, but should this be balanceShardUnique?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so actually. In v1 code this property gets passed to the overseer through CollectionAction.toString().toLower(Locale.ROOT) calls. So I've kept it lowercase in the v2 code to maintain parity with that.

Ideally the overseer would just be smart enough to ignore case when handling this property. (It's possible it does that already 🤷, though idk for sure.) But in any case I figured it was safest to use all lowercase as the v1 code has historically done.

@gerlowskija gerlowskija merged commit 7d71fe5 into apache:main Jun 8, 2023
1 of 4 checks passed
@gerlowskija gerlowskija deleted the SOLR-16398-balance-shard-unique-conversion branch June 8, 2023 18:18
gerlowskija added a commit that referenced this pull request Jun 17, 2023
This commit changes the v2 "balance-shard-unique" replica to be more
in line with the REST-ful design we're targeting for Solr's v2 APIs.

Following these changes, the v2 API now appears as: 

  `POST /api/collections/cName/balance-shard-unique {...}`

Although not shown above, the 'balance-shard-unique' command
specifier has also been removed from the request body.

This commit also converts the API to the new JAX-RS framework.
epugh pushed a commit that referenced this pull request Jun 21, 2023
This commit changes the v2 "balance-shard-unique" replica to be more
in line with the REST-ful design we're targeting for Solr's v2 APIs.

Following these changes, the v2 API now appears as: 

  `POST /api/collections/cName/balance-shard-unique {...}`

Although not shown above, the 'balance-shard-unique' command
specifier has also been removed from the request body.

This commit also converts the API to the new JAX-RS framework.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants