SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop #1332

stillalex · 2023-02-03T22:45:17Z

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

Description

List Aliases apis only.

Solution

List Aliases api responses:

curl -X GET 'http://localhost:8983/solr/admin/collections?action=LISTALIASES' | jq

{
  "responseHeader": {
    "status": 0,
    "QTime": 0
  },
  "aliases": {
    "testalias1": "collection1",
    "testalias2": "collection2,collection1"
  },
  "properties": {
    "testalias2": {
      "someKey": "someValue"
    }
  }
}

curl -X GET http://localhost:8983/api/aliases | jq

{
  "responseHeader": {
    "status": 0,
    "QTime": 1
  },
  "aliases": {
    "testalias1": "collection1",
    "testalias2": "collection2,collection1"
  },
  "properties": {
    "testalias2": {
      "someKey": "someValue"
    }
  }
}

curl -X GET http://localhost:8983/api/aliases/testalias2 | jq
{
  "responseHeader": {
    "status": 0,
    "QTime": 1
  },
  "name": "testalias2",
  "properties": {
    "someKey": "someValue"
  }
}

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

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

stillalex · 2023-02-03T22:45:42Z

@gerlowskija if you have some time, please take a look!
Just to clarify this is for list alias, to review and confirm the approach is ok, will extend the PR to other apis next.

gerlowskija · 2023-02-07T18:00:31Z

Hey, thanks for the PR Alex.

A lot of these have been blocked for awhile on SOLR-16531, but that's close enough to closed out that we should be good to start transitioning APIs again. I'll take a look shortly!

gerlowskija

list-aliases API looks great @stillalex. Left a comment or two but nothing of import.

You're planning to add a few other APIs to this PR it sounded like, or do you want to handle those in separate PRs? (Either approach works for me 🤷 )

gerlowskija · 2023-02-10T19:42:10Z

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

+ * </code> API.
+ */
+@Path("/aliases")
+public class GetAliasesAPI extends AdminAPIBase {


[0] Nitpick that you can def ignore, but I'd prefer the name ListAliasesAPI for this class instead. Right now Solr has no API to fetch information about a single alias, but it'd be a reasonable API to add at some point. And if that ever happens, it'd be hard for a reader to distinguish between GetAliasesAPI and GetAliasAPI without a double-take 😆

sure, renaming makes sense

solr/core/src/java/org/apache/solr/jersey/SolrRequestAuthorizer.java

stillalex · 2023-02-10T23:03:52Z

thanks for talking a look. I think I wanted to have a more complete PR with all apis. will try to add more to this, but if it turns into too much code, I will opt for splitting, it will also make the review a bit easier I guess.

.gitignore

gerlowskija · 2023-02-23T17:13:17Z

Small heads up for you @stillalex - I know it's tempting to force-push, but we've found that it can do weird things to Github comment history and make PRs just that bit harder to review. Obviously sometimes things get messy and you have to do what you have to do, but try to avoid those where you can at least please!

gerlowskija

Hi Alex - everything here looks great, but there's one tiny piece missing:

There's actually a v2 API in the class ClusterAPI that implements the list-alias functionality. We should delete that and remove its registration from CollectionsHandler.

It should only take a minute or two so I'll take a crack at that, and run the tests again. Other than that though, this all looks good to go!

gerlowskija · 2023-02-23T18:55:15Z

Alright, I fixed the ClusterAPI duplicate I mentioned, but in the process I found a few other things that were worth discussing here.

Specifically, the response that comes back from GET /api/aliases/aliasName isn't very useful. It's great that it returns the the free form properties. But there's not even a mention of the collections that the alias is a stand-in for.

$ curl -sk -X GET "http://localhost:8983/api/aliases/alias1" | jq '.'
{
  "responseHeader": {
    "status": 0,
    "QTime": 1
  },
  "name": "alias1",
  "properties": {}
}

AFAICT GET /api/aliases/aliasName is a brand new API, so we have some freedom to make the response format whatever we would like here. Would you be amenable to adding the collection-list to this response, or was there a reason you avoided that on your initial pass?

stillalex · 2023-02-23T19:45:18Z

AFAICT GET /api/aliases/aliasName is a brand new API, so we have some freedom to make the response format whatever we would like here. Would you be amenable to adding the collection-list to this response, or was there a reason you avoided that on your initial pass?

ouch, good catch! you are absolutely right, I missed this completely. for sure the collections list needs to be present. any preference on the format? what about 'collections' (similar to the name used when creating the alias)?

"name": "alias1",
"collections": "collection1",
"properties": {}

stillalex · 2023-02-23T20:19:23Z

@gerlowskija added the new entry in the response, a test for the new api and updated the guide. please take a look!

stillalex · 2023-02-27T19:07:59Z

@gerlowskija gentle ping. please take another look and let me know your thoughts

…p CRUD APIs

gerlowskija · 2023-03-06T18:56:29Z

Hey, sorry for the delay in circling back to this @stillalex - will take a look shortly with a mind to commit.

Careful about doing this. If I'd happened to push up some tweaks or something around the same time, they could've been blown away. It also does weird things with line-level review comments in the Github UI, that can be a bit of a pain when reviewing.

gerlowskija · 2023-03-06T19:19:34Z

Force-pushes also break this really nice UI feature that Github has of, "Only show me what's changed since the last time I posted a review", fwiw.

gerlowskija

LGTM; only one minor comment that I can cleanup prior to merge.

gerlowskija · 2023-03-06T19:23:39Z

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

+
+    final CoreContainer coreContainer = fetchAndValidateZooKeeperAwareCoreContainer();
+    ZkStateReader zkStateReader = coreContainer.getZkController().getZkStateReader();
+    // if someone calls listAliases, lets ensure we return an up to date response


[-0] Small nit: copy/paste issue with 'listAliases'.

Relatedly though, maybe this code should be pulled into a private helper method that both getAliasByName and getAliases can call, to cut down on duplication.

stillalex · 2023-03-07T23:18:17Z

thank you @gerlowskija for the help in getting this merged!

gerlowskija reviewed Feb 10, 2023

View reviewed changes

stillalex force-pushed the SOLR-16393 branch from b23f43e to 1a85af1 Compare February 10, 2023 23:52

gerlowskija reviewed Feb 15, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

stillalex force-pushed the SOLR-16393 branch from 1a85af1 to 23b94bd Compare February 22, 2023 17:52

gerlowskija requested changes Feb 23, 2023

View reviewed changes

stillalex and others added 6 commits March 2, 2023 07:13

SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-pro…

29a3c41

…p CRUD APIs

Minor renames, remove duplicate ClusterAPI definition

ff1ab0e

adding collection list to single alias response

a432c83

fix for single alias and test

11e70c6

updated guide

a3a18c6

removed test for the api that was removed

edac8f5

stillalex force-pushed the SOLR-16393 branch from 82eeab0 to edac8f5 Compare March 2, 2023 15:14

removing more old cluster tests

0c548fb

gerlowskija approved these changes Mar 6, 2023

View reviewed changes

gerlowskija added 2 commits March 6, 2023 14:33

Extract duplicate code

3bd5173

Changes.txt entry

71dc0c1

gerlowskija merged commit ef49422 into apache:main Mar 6, 2023

stillalex deleted the SOLR-16393 branch March 7, 2023 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop #1332

SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop #1332

stillalex commented Feb 3, 2023 •

edited

Loading

stillalex commented Feb 3, 2023 •

edited

Loading

gerlowskija commented Feb 7, 2023

gerlowskija left a comment

gerlowskija Feb 10, 2023

stillalex Feb 10, 2023

stillalex commented Feb 10, 2023

gerlowskija commented Feb 23, 2023

gerlowskija left a comment

gerlowskija commented Feb 23, 2023

stillalex commented Feb 23, 2023

stillalex commented Feb 23, 2023

stillalex commented Feb 27, 2023

gerlowskija commented Mar 6, 2023

gerlowskija commented Mar 6, 2023

gerlowskija left a comment

gerlowskija Mar 6, 2023

stillalex Mar 6, 2023

stillalex commented Mar 7, 2023

SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop #1332

SOLR-16393 Cosmetic, REST-fulness improvements to v2 alias, alias-prop #1332

Conversation

stillalex commented Feb 3, 2023 • edited Loading

Description

Solution

Tests

Checklist

stillalex commented Feb 3, 2023 • edited Loading

gerlowskija commented Feb 7, 2023

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija Feb 10, 2023

Choose a reason for hiding this comment

stillalex Feb 10, 2023

Choose a reason for hiding this comment

stillalex commented Feb 10, 2023

gerlowskija commented Feb 23, 2023

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija commented Feb 23, 2023

stillalex commented Feb 23, 2023

stillalex commented Feb 23, 2023

stillalex commented Feb 27, 2023

gerlowskija commented Mar 6, 2023

gerlowskija commented Mar 6, 2023

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija Mar 6, 2023

Choose a reason for hiding this comment

stillalex Mar 6, 2023

Choose a reason for hiding this comment

stillalex commented Mar 7, 2023

stillalex commented Feb 3, 2023 •

edited

Loading

stillalex commented Feb 3, 2023 •

edited

Loading