SOLR-16372: Migrate collection listing, deletion to JAX-RS #1412
Conversation
In edition to migrating this one API, this PR establishes a pattern for other overseer-based APIs whose responses aggregate the responses from any and all sub-requests made by the overseer.
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
|
Alright, this should be ready to go. I'll plan on merging midweek and then backporting later on pending any feedback from folks. |
|
|
||
| /** Customizes the ObjectMapper settings used for serialization/deserialization in Jersey */ | ||
| @SuppressWarnings("rawtypes") |
There was a problem hiding this comment.
Did you try to fix this or too tricky? These are annoying
There was a problem hiding this comment.
Yeah, I looked into it a bit but couldn't quite figure out the right syntax unfortunately
| @Override | ||
| public void serialize(NamedList value, JsonGenerator gen, SerializerProvider provider) | ||
| throws IOException { | ||
| gen.writeObject(value.asShallowMap()); |
There was a problem hiding this comment.
I suspect if benchmarks are done, this is going to be revisited to be more efficient. That NamedList.shallowMap looks trivial but the implementations of several of its methods call NamedList.asMap which is not. Fine for now I guess.
CC @noblepaul
There was a problem hiding this comment.
NamedList.shallowMap looks trivial but the implementations of several of its methods call NamedList.asMap which is not
Yeah, I'd picked the shallow-map approach because it looked reasonably trivial, but point well taken about the asMap usage. I suspect there's something that could be done with the MapWriter interface, but I couldn't get it working and eventually settled for what you see here.
| public class SubResponseAccumulatingJerseyResponse extends SolrJerseyResponse { | ||
|
|
||
| @JsonProperty("requestid") | ||
| public String requestId; |
There was a problem hiding this comment.
Isn't the requestId here the same as "rid" which is only used in logging? But maybe not because it's a response JSON property?
There was a problem hiding this comment.
Not 'rid' related at all, though the overlap in terminology is definitely confusing and makes it seem that way.
This is about the async parameter that some collection-admin APIs take to run commands asynchronously.
Today, if you make a collection-admin request with an async param, Solr will return a mostly blank response with only this 'requestid' field set (echoing back the async value you passed in).
There was a problem hiding this comment.
I found "requestid" somewhat overloaded as a term as well.... operation_id?
There was a problem hiding this comment.
I think that Solr spits out 'requestid' here because that's the name of the parameter that v1 REQUESTSTATUS has taken, historically.
Definitely something worth cleaning up at some point. (Personally my preference would be for something more explicit, like asynchronous-request-id, but admittedly that's probably too verbose for most people's tastes.) I'd be onboard with any option that helped requestid be less overloaded.
That said - definitely beyond the scope of this ticket to make any field naming changes, unfortunately. Changing the response format would impact v1 APIs as well, have backcompat implications, etc.
There was a problem hiding this comment.
For these types of feedback, what's the best way foreward? Should we create a JIRA for each one of these and make it be related to the current migration related JIRA, (in this case SOLR-16372)? If so, do you want to do them as the person moving the current issue forward, or me to do it as "the kibitzing person coming up with ideas" person? I don't want to lose these ideas improvements ;-)
| // Only required properties provided | ||
| { | ||
| final ZkNodeProps message = | ||
| DeleteCollectionAPI.createRemoteMessage("someCollName", null, null); |
There was a problem hiding this comment.
Sigh... one day if we ever want to remove the Overseer fully, I see here an ever increasing number of tests that assume its existence. Yeah we've spoken about this, just reading this test with a heavy sigh.
The endpoints themselves remain the same, other than moving to the JAX-RS framework.
The endpoints themselves remain the same, other than moving to the JAX-RS framework.
https://issues.apache.org/jira/browse/SOLR-16372
Description
Solr is in the process of migrating its v2 APIs over to a JAX-RS framework. This gives us a more feature-rich framework for expressing APIs, makes it easier for the APIs themselves to be more REST-ful and standardized, and also makes it easier for Solr to integrate with tooling such as OpenAPI, which can be used to autogenerate client bindings for a variety of different languages.
But many non-JAX-RS APIs remain.
Solution
This PR introduces a baby-step in the right direction by migrating two additional APIs to JAX-RS: the "list collections" and "delete collection" APIs.
Tests
See
DeleteCollectionAPITest. Existing v2 tests continue to pass.Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.