SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful #2144

iamsanjay · 2023-12-11T11:39:17Z

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

Description

V2(JAX-RS) Endpoints for core admin op "command-status".

Solution

This commit changes the v2 "command-status" API 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: GET /api/cores/coreName/command-status/123

The old v2 api class RequestCoreCommandStatusAPI.java has also been removed.

Tests

Three test cases are added:

Completed task response and code.
Failed task response and code.
Running task response and code.

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

gerlowskija · 2024-01-02T16:25:50Z

Hey - sorry for the delay in reviewing this over the holidays. Taking a look now!

gerlowskija

Looks great, thanks for converting another API!

There's one problem here I think we need to sort out: the /cores/command-status/<id> path is ambiguous with some other Solr APIs. (I left a comment on RequestCoreCommandStatusApi with more details, but don't have a clear solution in mind yet. Lmk what you think!)

My other comments are less consequential.

Thanks again!

gerlowskija · 2024-01-02T19:10:53Z

solr/CHANGES.txt

@@ -124,6 +124,9 @@ Improvements

 * SOLR-17063: Do not retain log param references in LogWatcher (Michael Gibney)

+* SOLR-16397: The command-status v2 endpoint has been updated to be more REST-ful.


[0] Solr has a similar API for requesting the status of collection level operations, so it might be worth clarifying here which one was updated.

gerlowskija · 2024-01-02T19:11:26Z

solr/api/src/java/org/apache/solr/client/api/endpoint/RequestCoreCommandStatusApi.java

+/**
+ * V2 API for checking the status of a core-level asynchronous command.
+ *
+ * <p>This API (GET /api/cores/command-status/someId is analogous to the v1


[0] Missing ')' after "someId"

gerlowskija · 2024-01-02T19:12:01Z

solr/api/src/java/org/apache/solr/client/api/endpoint/RequestCoreCommandStatusApi.java

+ * /admin/cores?action=REQUESTSTATUS command. It is not to be confused with the more robust
+ * asynchronous command support offered under the v2 `/cluster/command-status` path (or the
+ * corresponding v1 path `/solr/admin/collections?action=REQUESTSTATUS`). Async support at the core
+ * level differs in that command IDs are local to individual Solr nodes and are not persisted across


[+1] This is a great call-out.

gerlowskija · 2024-01-02T19:27:16Z

solr/api/src/java/org/apache/solr/client/api/endpoint/RequestCoreCommandStatusApi.java

+ *
+ * @see org.apache.solr.client.api.model.RequestCoreCommandStatusResponseBody
+ */
+@Path("/cores/command-status/")


[-1] Hmmm, I don't think this API path will work - its ambiguous with other Solr APIs

e.g. /cores/command-status/select could either be an attempt to lookup the status of a core-level operation that used "select" as its async-id, or it could be an attempt to query the data in a core called "command-status".

I have some reservations about the existing path for this API (e.g. /cores/myCore/command-status/123), which avoids the ambiguity by requiring a specific core name. But maybe we should just keep the core name in the path here to avoid the ambiguity. Or maybe someone else can think up a third option that avoids the ambiguity while saving us from needing to provide a path-param that may not always make sense...

Wdyt?

Valid point! Can we have a new path for such operations, /api/operations/command-status/123.if not, then I will just put back the coreName in it.

Mm, I'd love /api/operations/command-status if it was a unified place for all of the asynchonous/long-running commands Solr let you run. But Solr is pretty far away from that, unfortunately. Solr has at least three different ways that it tracks asynchronous operations (core-operations, collection-operations, and "cancellable tasks"), each with its own APIs and underlying implementation.

Let's just go with putting the coreName back in for now I guess 😦

I was testing the api with coreName back in it. But It works completely fine even if no such core exists! Is that expected?

I'm not sure about "expected", but it's definitely odd haha.

If we wanted, we could have it validate the core and throw a SolrException(ErrorCode.NOT_FOUND, "...") if the specified core doesn't exist?

Or maybe someone else can think up a third option that avoids the ambiguity while saving us from needing to provide a path-param that may not always make sense...

Some v2 APIs start their path with "node" to indicate that the request is relevant only to the receiving node. Like /api/node/health (which reports the health of the receiving node) or /api/node/key (which returns the public encryption key of the receiving node).

Maybe that's how we solve our path problem here - we could pick a path starting with /api/node instead of trying to squeeze it under /cores or elsewhere.

It fits well conceptually IMO. The status of these commands is already node-specific, i.e. a "request-status" to 'NodeA' can only tell you about tasks run on 'NodeA'. So an API like /api/nodes/commands/requestId makes a lot of sense here IMO. And it goes well with the API we'll eventually have for tracking cluster-wide tasks/commands: /api/cluster/commands/someRequestId

Wdyt? If you like the approach I can add the changes in here. Sorry for waffling on this so many times!

Oops, wanted to tag you: @iamsanjay

@gerlowskija iterations are perfect. I can totally add the change whatever we will decide on.
Regarding choosing path starting with /api/node. Just a thought that the concept of node is more SolrCloud, and here we are dealing with request status command at core admin level, though technically node and core are the same thing. However, introducing node meaning adding another concept for user who just started using Solr at core level. Let me know if there is any flaw in my thinking about nodes vs cores.

Just a thought that the concept of node is more SolrCloud, and here we are dealing with request status command at core admin level

I see your point. "Node" does kindof imply a distributed setup in a way that might be odd for folks that only run a single Solr process.

That said, I'd hope that folks that run a single instance of Solr in "standalone" mode would understand that Solr does allow you to network multiple processes (even if they're not using that capability themselves). So hopefully it wouldn't be too confusing?

(Even "standalone" mode has features built on running multiple instances, fwiw. e.g. You can do leader-follower replication from cores on different Solr processes/nodes. "Standalone" is kindof a bad name in that sense haha)

Anyway, I definitely see your concerns, but IMO /api/node/commands/someRequestId is still among our best options. It'd probably be my pick personally, but I support sticking with /api/cores/coreName/commands/someRequestId if you like that one better. Just lmk in your reply which one you want to go with, and whether you want to handle the changes (or whether I should)!

Thanks again to being flexible to the back-and-forth on this; definitely gives us a better end product!

gerlowskija · 2024-01-02T19:37:00Z

solr/core/src/test/org/apache/solr/handler/admin/api/RequestCoreCommandStatusTest.java

+import org.junit.Test;
+
+/** Test for {@link RequestCoreCommandStatus}. */
+public class RequestCoreCommandStatusTest extends JerseyTest {


[-0] I can't recall right now, but in a prior PR I ran into an issue with how Solr's test-framework ran JerseyTest-based test classes.

Let me dig into this a bit more to try to remember what the issue was, but we might have to restructure this to be more of a pure unit test like you wrote in #2128

I saw the build passing with this test. I am hoping we will be able to add this to main.
Because I like JerseyTest as they test the real end-points rather calling the method. Wdyt?

Agreed - I also really like the "level" that JerseyTest allows us to validate - being able to test that the HTTP path, request body, etc. match what you expect is really valuable. And I've pulled the code down locally and agree that the test passes as-is. It's a good test 👍

The problem is the base-class, JerseyTest.

Generally in Solr's test suite, we want to support users running any number of tests in parallel (or even running the same test 'N' times in parallel) without any conflicts. Solr's base classes (SolrTestCase, SolrTestCaseJ4, etc.) have a good bit of logic built into them to support this: locking around how ports are allocated when starting test servers, ensuring system properties are cleared in between tests to avoid any spillover, tracking "Closable" resources to detect memory leaks, etc.

JerseyTest doesn't provide a lot of these "good citizen" guarantees though. I accidentally broke the build earlier this year when adding a JerseyTest subclass for another API. (I'm hazy on the specifics, but I think that whenever two JerseyTest-based tests ran at the same time they ended up "choosing" the same port for the test server and failing.)

It's definitely fixable - we could probably look at what JerseyTest is actually doing and come up with a public class SolrJerseyTest extends SolrTestCase that brings the best of both worlds together. But that doesn't exist yet, so I've been trying to use JerseyTest sparingly for now.

See PR review comments for context; this change allows us to avoid a weird 'core' path-parameter that isn't actually required by the API implementation. Considering these as 'node' async tasks also fits better conceptually. Also reimplements the unit tests to avoid the JerseyTest base.

gerlowskija · 2024-01-24T22:15:59Z

@iamsanjay - I would love to see this make it into 9.5, so I took the liberty of making a few of the changes that were outstanding from above. Namely:

Changing the API to be /api/node/commands/<someId>, to avoid the unused-coreName problem.
Restructuring your unit tests to avoid use of JerseyTest.

As we discussed this already above I don't expect you'll object to any of this, but if you do lmk and I'm happy to revert my recent commits on the branch while we discuss. But otherwise, my next steps are to update CHANGES.txt, and get the tests passing so we can merge this soon!

This commit changes the v2 "requeststatus" (core level) API 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: `GET /api/node/commands/someCommandId` --------- Co-authored-by: iamsanjay <[email protected]>

* main: (27 commits) Update protected-branches to include branch_9_5 (apache#2211) SOLR-16397: Tweak v2 'REQUESTSTATUS' API to be more REST-ful (apache#2144) SOLR-17120: handle null value when merging partials (apache#2214) SOLR-17119: Fix exception swallowing in /cluster/plugins (apache#2202) SOLR-15960 Cut over System.getProperty() to EnvUtils for modules (apache#2193) Final fix for node problems (apache#2208) SOLR-16397: Fix warning in merge-indices docs Fix nodeSetup, use node distBaseUrl instead of registry (apache#2208) Add next minor version 9.6.0 SOLR-17089: Upgrade Jersey to 3.1.5 (apache#2178) solr-ref-guide: fix typo in result-clustering.adoc (apache#2210) SOLR-17074: Fixed not correctly escaped quote in bin/solr script (apache#2200) SOLR-15960: Rename getProp as getProperty (apache#2194) Add npmRegistry for nodeSetup as well (apache#2208) Give NPM registry option for downloading node tools (apache#2208) SOLR-17116: Fix INSTALLSHARDDATA async reporting (apache#2188) SOLR-17066: Replace 'data store' term in code and docs (apache#2201) SOLR-17121: Fix SchemaCodecFactory to get PostingsFormat and DocValues from field. (apache#2206) Sync CHANGES for 9.4.1 Add bugfix version 9.4.1 ...

iamsanjay · 2024-01-31T13:15:23Z

@iamsanjay - I would love to see this make it into 9.5, so I took the liberty of making a few of the changes that were outstanding from above. Namely:

Thank you so much! Moving on to the new one, going to submit a PR, soon!

iamsanjay added 4 commits December 11, 2023 16:41

SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful

71883d8

Merge branch 'main' into SOLR_16461-v2-requeststatus-CoreAdminOps

0a223d5

SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful

b9002a5

Reformat code for Test class

fac3b0f

gerlowskija reviewed Jan 2, 2024

View reviewed changes

iamsanjay and others added 5 commits January 5, 2024 12:16

SOLR-16397: Merge main branch, updated changes.txt and reformat comment

e687d84

Merge main branch

b6387a8

Added the core name in the v2 path

0dbb20a

Merge branch 'main' into SOLR_16397-v2-requeststatus-CoreAdminOps

bab29af

github-actions bot added documentation Improvements or additions to documentation api labels Jan 24, 2024

Update API in docs

1fb0f69

gerlowskija merged commit 06a3751 into apache:main Jan 25, 2024
3 checks passed

psalagnac mentioned this pull request Mar 4, 2024

SOLR-17160: Time based tracking of core admin requests with Caffeine cache #2304

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful #2144

SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful #2144

iamsanjay commented Dec 11, 2023 •

edited by gerlowskija

Loading

gerlowskija commented Jan 2, 2024

gerlowskija left a comment

gerlowskija Jan 2, 2024

gerlowskija Jan 2, 2024

gerlowskija Jan 2, 2024

gerlowskija Jan 2, 2024

iamsanjay Jan 5, 2024

gerlowskija Jan 5, 2024

iamsanjay Jan 6, 2024

gerlowskija Jan 12, 2024 •

edited

Loading

gerlowskija Jan 12, 2024

iamsanjay Jan 14, 2024

gerlowskija Jan 16, 2024

gerlowskija Jan 2, 2024

iamsanjay Jan 14, 2024

gerlowskija Jan 16, 2024

gerlowskija commented Jan 24, 2024

iamsanjay commented Jan 31, 2024

		@@ -124,6 +124,9 @@ Improvements

		* SOLR-17063: Do not retain log param references in LogWatcher (Michael Gibney)

		* SOLR-16397: The command-status v2 endpoint has been updated to be more REST-ful.

SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful #2144

SOLR-16397: Modify v2 'REQUESTSTATUS' API to be more REST-ful #2144

Conversation

iamsanjay commented Dec 11, 2023 • edited by gerlowskija Loading

Description

Solution

Tests

Checklist

gerlowskija commented Jan 2, 2024

gerlowskija left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerlowskija commented Jan 24, 2024

iamsanjay commented Jan 31, 2024

iamsanjay commented Dec 11, 2023 •

edited by gerlowskija

Loading

gerlowskija Jan 12, 2024 •

edited

Loading