cugarte commented on PR #2788:
URL: https://github.com/apache/solr/pull/2788#issuecomment-2451974121
Thank you for the detailed feedback, @gerlowskija! I've updated the code to
reflect most of the changes you suggested. Specifically,
- Merged all of the previously new classes in `o.a.s.client.api.endpoint`
into a new class `ClusterPropertyApis` and all of the previously new classes
`o.a.s.handler.admin.api` into a new class `ClusterProperty` (updated to
extend/use `AdminAPIBase`),
- Removed all of the previously new classes that were merged above,
- Updated the v1 code in `CollectionsHandler` to use the new
`ClusterProperty` APIs,
- Removed the old v2 code in `ClusterAPI` (both `setObjProperty` which you
suggested and `setProperty` which I had originally left unchanged (used the v1
API code path) as both were v2 experimental ClusterProps APIs),
- Moved registration of the new code from `CoreContainer` to
`CollectionsHandler`,
- Updated the v2 JAX-RS Fetch Single ClusterProp API (`getClusterProperty`)
to return a 404 instead of a 400, and
- Added the missing `@PermissionName` annotations (now in `ClusterProperty`).
A couple of code suggestions are outstanding:
> [-0] These tests seem like a good candidate for using the SolrJ classes
generated by your API definitions (i.e.
`o.a.s.client.solrj.request.ClusterPropertiesApi`).
>
> Doing so feels a bit circular (i.e. any bugs in the JAX-RS annotations
would get propagated to the generated code and might not be caught)...but it'd
help the tests to scan a lot nicer IMO, and it'd get us coverage for the SolrJ
classes to boot, so it sees like a worthwhile tradeoff IMO. Wdyt?
I hadn't thought of this earlier, but think it's a good idea as it would let
us test the SolrJ classes. I don't know enough about the code generation or
how annotation bugs might lead to masking errors - perhaps it would be helpful
to have a specific set of tests that checks an underlying API in multiple ways
(using the SolrJ APIs and HttpClient calls or something similar?) but
generally let test code just use the SolrJ APIs? In any event, I'm running
into a couple of problems here that I think are related to code generation.
Problem 1: I added one new unit test
(`testClusterPropertyFetchNonExistentPropertySolrJ`, left the others unchanged)
using a generated SolrJ API. I didn't see any other unit tests that used the
other generated classes from `o.a.s.client.solrj.request` so am guessing as to
the proper way to use generated client code. This particular test is meant to
return an error, and I believe it triggers an error when the request is handled
server-side, but the invocation of the client API (`new
ClusterPropertiesApi.GetClusterProperty("ext.clusterPropThatDoesNotExist").process(client)`)
does not throw an exception as I had expected, so the unit test fails. Not
sure how to check for errors otherwise.
>Hmm - I think you should be able to nuke
`SetNestedClusterPropertyRequestBody` altogether, and replace it in the method
signature with `Map<String, Object>`
Problem 2: when I did this, the generated code below (in
`o.a.s.client.solrj.request.ClusterPropertiesApi`) could not be compiled:
```java
this.requestBody = new Map<String, Object>();
```
as Map is an abstract class. Replacing
`SetNestedClusterPropertyRequestBody` with `HashMap<String, Object>` yields the
exact same issue (the generated code tries to instantiate a `Map`). I left
this part unchanged.
Question 1: while I also prefer having related calls be in the same class,
this has resulted in two similarly named classes that could be confusing:
`o.a.s.handler.admin.api.ClusterProperty` and
`org.apache.solr.common.cloud.ClusterProperties`. The latter implements the
interaction with ZooKeeper. Is this ok, or is there a convention I should
follow?
TODOs:
- Updating `dev-docs/v2-api-conventions.adoc` convention on how v2 APIs
should return not found errors.
- Updating documentation on the v2 APIs.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
