gerlowskija commented on code in PR #4826:
URL: https://github.com/apache/solr/pull/4826#discussion_r3926161892
##########
dev-docs/v2-api-conventions.adoc:
##########
@@ -139,3 +139,115 @@ In these cases, developers may:
AddFieldOperation requestBody)
throws Exception;
```
+
+== Response POJOs
+
+Every v2 response body extends `SolrJerseyResponse`, which provides
`responseHeader` and `error`.
+Beyond that base, pick the narrowest existing response type that fits before
writing a new one:
+
+* `SolrJerseyResponse` - plain success/error response with no extra data. The
default for simple mutations.
+* `AsyncJerseyResponse extends SolrJerseyResponse` - adds a `requestId` field.
Use this (or a subclass of it) for any API that supports the `async`
request-body parameter, since
`submitRemoteMessageAndHandleAsync`/`handlePotentiallyAsynchronousTask`
populate `requestId` automatically when an async id is present.
+* `SubResponseAccumulatingJerseyResponse extends AsyncJerseyResponse` - adds
`successfulSubResponsesByNodeName`, `failedSubResponsesByNodeName`, and
`warning`. Use this for Overseer-driven APIs that fan out to multiple
nodes/replicas (e.g. `CreateShard`, `DeleteShard`) -
`AdminAPIBase.submitRemoteMessageAndHandleResponse` populates these fields from
the Overseer's `success`/`failure`/`warning` NamedList entries automatically.
+* `FlexibleSolrJerseyResponse extends SolrJerseyResponse` - adds
`@JsonAnyGetter`/`@JsonAnySetter`-backed dynamic *top-level* properties, for
APIs whose entire response shape is genuinely open-ended (e.g.
`SchemaDesigner`, the `Select` query API). This is different from the
dynamic-request-POJO pattern above: it's for responses, and the dynamism
applies to the whole top level rather than one field.
+
+If an API returns specific, known additional data beyond these bases (e.g.
timing information, computed ranges, a resource's status fields), extend the
appropriate base with typed `@JsonProperty` fields rather than reaching for
`FlexibleSolrJerseyResponse` - see `SplitShardResponse.timing`,
`SplitCoreResponse.ranges`, or `CollectionStatusResponse` for examples.
+Don't settle for a bare `SolrJerseyResponse` if the underlying operation
actually produces more than a bare success/error - a v2 JSON caller has no
other way to get that data, since (unlike v1) nothing else in the response
pipeline will surface it (see the async caveat below for why this matters in
practice).
+
+== Async Task Handling
+
+Solr has two distinct, non-interchangeable mechanisms for handling the `async`
request parameter, depending on which base class the API extends:
+
+* **Core-level (`CoreAdminAPIBase`)**:
`handlePotentiallyAsynchronousTask(response, coreName, taskId, actionName,
supplier)`. If `taskId` is null, the supplier runs inline and its result is
returned directly. If non-null, the supplier is wrapped in a
`CoreAdminAsyncTracker.TaskObject` and submitted to `coreAdminAsyncTracker`,
which tracks status for later `REQUESTSTATUS` polling. Because the supplier is
a `Supplier<T>`, it cannot throw checked exceptions directly - wrap them in
`CoreAdminAPIBase.CoreAdminAPIBaseException` and rethrow, which
`handlePotentiallyAsynchronousTask` unwraps back to the original checked
exception for the caller.
+* **Collection-level (`AdminAPIBase`)**:
`submitRemoteMessageAndHandleAsync`/`submitRemoteMessageAndHandleResponse(response,
action, remoteMessage, asyncId[, timeoutMs])`. The `asyncId` is baked into the
submitted `ZkNodeProps` message and handled by the Overseer's own
async-tracking machinery; `response.requestId` is populated automatically when
`asyncId` is non-null. A timeout-aware overload exists for APIs (like shard
split) that legitimately need longer than
`CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT` to complete.
+
+Only wrap the branches of an API that are actually meant to support async
execution.
+A synchronous-by-design sub-operation (e.g. a "dry run" or "compute
recommendations" branch that happens to share a method with the real mutating
operation) should bypass async handling entirely rather than being routed
through it - see the `async` caveat under "Relationship Between V1 and V2
Implementations" below for a concrete failure mode this avoids.
+
+== Relationship Between V1 and V2 Implementations
+
+Most v2 APIs have a corresponding legacy v1 API (e.g.
`/admin/cores?action=RELOAD` backs `POST /api/cores/coreName/reload`).
Review Comment:
[0] This section seems like it might be better placed in
`dev-docs/apis.adoc`. There's a section there where we already go through how
to write a JAX-RS API step-by-step. The rationale info here would be perfect
as a sub-bullet in that step-by-step IMO.
##########
dev-docs/v2-api-conventions.adoc:
##########
@@ -139,3 +139,115 @@ In these cases, developers may:
AddFieldOperation requestBody)
throws Exception;
```
+
+== Response POJOs
+
+Every v2 response body extends `SolrJerseyResponse`, which provides
`responseHeader` and `error`.
Review Comment:
Not quite true, there are exceptions where we have a JAX-RS method return
'StreamingOutput'. But maybe it's just being pedantic to mention that here...
##########
dev-docs/v2-api-conventions.adoc:
##########
@@ -139,3 +139,115 @@ In these cases, developers may:
AddFieldOperation requestBody)
throws Exception;
```
+
+== Response POJOs
+
+Every v2 response body extends `SolrJerseyResponse`, which provides
`responseHeader` and `error`.
+Beyond that base, pick the narrowest existing response type that fits before
writing a new one:
+
+* `SolrJerseyResponse` - plain success/error response with no extra data. The
default for simple mutations.
+* `AsyncJerseyResponse extends SolrJerseyResponse` - adds a `requestId` field.
Use this (or a subclass of it) for any API that supports the `async`
request-body parameter, since
`submitRemoteMessageAndHandleAsync`/`handlePotentiallyAsynchronousTask`
populate `requestId` automatically when an async id is present.
+* `SubResponseAccumulatingJerseyResponse extends AsyncJerseyResponse` - adds
`successfulSubResponsesByNodeName`, `failedSubResponsesByNodeName`, and
`warning`. Use this for Overseer-driven APIs that fan out to multiple
nodes/replicas (e.g. `CreateShard`, `DeleteShard`) -
`AdminAPIBase.submitRemoteMessageAndHandleResponse` populates these fields from
the Overseer's `success`/`failure`/`warning` NamedList entries automatically.
+* `FlexibleSolrJerseyResponse extends SolrJerseyResponse` - adds
`@JsonAnyGetter`/`@JsonAnySetter`-backed dynamic *top-level* properties, for
APIs whose entire response shape is genuinely open-ended (e.g.
`SchemaDesigner`, the `Select` query API). This is different from the
dynamic-request-POJO pattern above: it's for responses, and the dynamism
applies to the whole top level rather than one field.
+
+If an API returns specific, known additional data beyond these bases (e.g.
timing information, computed ranges, a resource's status fields), extend the
appropriate base with typed `@JsonProperty` fields rather than reaching for
`FlexibleSolrJerseyResponse` - see `SplitShardResponse.timing`,
`SplitCoreResponse.ranges`, or `CollectionStatusResponse` for examples.
+Don't settle for a bare `SolrJerseyResponse` if the underlying operation
actually produces more than a bare success/error - a v2 JSON caller has no
other way to get that data, since (unlike v1) nothing else in the response
pipeline will surface it (see the async caveat below for why this matters in
practice).
+
+== Async Task Handling
+
+Solr has two distinct, non-interchangeable mechanisms for handling the `async`
request parameter, depending on which base class the API extends:
+
+* **Core-level (`CoreAdminAPIBase`)**:
`handlePotentiallyAsynchronousTask(response, coreName, taskId, actionName,
supplier)`. If `taskId` is null, the supplier runs inline and its result is
returned directly. If non-null, the supplier is wrapped in a
`CoreAdminAsyncTracker.TaskObject` and submitted to `coreAdminAsyncTracker`,
which tracks status for later `REQUESTSTATUS` polling. Because the supplier is
a `Supplier<T>`, it cannot throw checked exceptions directly - wrap them in
`CoreAdminAPIBase.CoreAdminAPIBaseException` and rethrow, which
`handlePotentiallyAsynchronousTask` unwraps back to the original checked
exception for the caller.
Review Comment:
[-0] This detail is great, but it feels pretty far from this document's
original purpose of covering what our v2 APIs should look like cosmetically and
JAX-RS implementation details.
Or to put it a different way: there's lots of detail that might be helpful
to a person writing a v2 API. The distinction between the overseer and David's
"Distributed Command Processing" stuff. CoreContainer vs. SolrCore API
registration. How clusterstate is represented in ZooKeeper. But if we're
going to jam all that stuff in here, this doc will become an absolute mess!!
I'm not saying it doesn't deserve to be documented, but it might belong
better in a different doc?
##########
dev-docs/v2-api-conventions.adoc:
##########
@@ -139,3 +139,115 @@ In these cases, developers may:
AddFieldOperation requestBody)
throws Exception;
```
+
+== Response POJOs
+
+Every v2 response body extends `SolrJerseyResponse`, which provides
`responseHeader` and `error`.
+Beyond that base, pick the narrowest existing response type that fits before
writing a new one:
+
+* `SolrJerseyResponse` - plain success/error response with no extra data. The
default for simple mutations.
+* `AsyncJerseyResponse extends SolrJerseyResponse` - adds a `requestId` field.
Use this (or a subclass of it) for any API that supports the `async`
request-body parameter, since
`submitRemoteMessageAndHandleAsync`/`handlePotentiallyAsynchronousTask`
populate `requestId` automatically when an async id is present.
+* `SubResponseAccumulatingJerseyResponse extends AsyncJerseyResponse` - adds
`successfulSubResponsesByNodeName`, `failedSubResponsesByNodeName`, and
`warning`. Use this for Overseer-driven APIs that fan out to multiple
nodes/replicas (e.g. `CreateShard`, `DeleteShard`) -
`AdminAPIBase.submitRemoteMessageAndHandleResponse` populates these fields from
the Overseer's `success`/`failure`/`warning` NamedList entries automatically.
+* `FlexibleSolrJerseyResponse extends SolrJerseyResponse` - adds
`@JsonAnyGetter`/`@JsonAnySetter`-backed dynamic *top-level* properties, for
APIs whose entire response shape is genuinely open-ended (e.g.
`SchemaDesigner`, the `Select` query API). This is different from the
dynamic-request-POJO pattern above: it's for responses, and the dynamism
applies to the whole top level rather than one field.
+
+If an API returns specific, known additional data beyond these bases (e.g.
timing information, computed ranges, a resource's status fields), extend the
appropriate base with typed `@JsonProperty` fields rather than reaching for
`FlexibleSolrJerseyResponse` - see `SplitShardResponse.timing`,
`SplitCoreResponse.ranges`, or `CollectionStatusResponse` for examples.
+Don't settle for a bare `SolrJerseyResponse` if the underlying operation
actually produces more than a bare success/error - a v2 JSON caller has no
other way to get that data, since (unlike v1) nothing else in the response
pipeline will surface it (see the async caveat below for why this matters in
practice).
+
+== Async Task Handling
+
+Solr has two distinct, non-interchangeable mechanisms for handling the `async`
request parameter, depending on which base class the API extends:
+
+* **Core-level (`CoreAdminAPIBase`)**:
`handlePotentiallyAsynchronousTask(response, coreName, taskId, actionName,
supplier)`. If `taskId` is null, the supplier runs inline and its result is
returned directly. If non-null, the supplier is wrapped in a
`CoreAdminAsyncTracker.TaskObject` and submitted to `coreAdminAsyncTracker`,
which tracks status for later `REQUESTSTATUS` polling. Because the supplier is
a `Supplier<T>`, it cannot throw checked exceptions directly - wrap them in
`CoreAdminAPIBase.CoreAdminAPIBaseException` and rethrow, which
`handlePotentiallyAsynchronousTask` unwraps back to the original checked
exception for the caller.
+* **Collection-level (`AdminAPIBase`)**:
`submitRemoteMessageAndHandleAsync`/`submitRemoteMessageAndHandleResponse(response,
action, remoteMessage, asyncId[, timeoutMs])`. The `asyncId` is baked into the
submitted `ZkNodeProps` message and handled by the Overseer's own
async-tracking machinery; `response.requestId` is populated automatically when
`asyncId` is non-null. A timeout-aware overload exists for APIs (like shard
split) that legitimately need longer than
`CollectionsHandler.DEFAULT_COLLECTION_OP_TIMEOUT` to complete.
+
+Only wrap the branches of an API that are actually meant to support async
execution.
+A synchronous-by-design sub-operation (e.g. a "dry run" or "compute
recommendations" branch that happens to share a method with the real mutating
operation) should bypass async handling entirely rather than being routed
through it - see the `async` caveat under "Relationship Between V1 and V2
Implementations" below for a concrete failure mode this avoids.
+
+== Relationship Between V1 and V2 Implementations
+
+Most v2 APIs have a corresponding legacy v1 API (e.g.
`/admin/cores?action=RELOAD` backs `POST /api/cores/coreName/reload`).
+Where both exist, **the actual business logic should live in the v2
implementation class, and the v1 handler should delegate to it** - not the
other way around.
+This convention exists for a few reasons:
+
+1. It makes it easier to delete the v1 code down the road.
+2. It prevents the v2 code from "falling behind" or being forgotten when query
params or API functionality changes, since v1 traffic continuously exercises
the same v2 code path.
+3. It gives the v2 endpoint test coverage "for free" - most of Solr's existing
test suite targets v1 APIs, so having v1 call v2 gives confidence that the v2
endpoint works correctly even in the absence of direct v2-specific tests.
+
+How this delegation is implemented differs depending on whether the API
executes synchronously or is processed by the Overseer.
+
+=== Synchronous (core-level) APIs
Review Comment:
There's a big difference between "core-level" APIs and CoreAdminAPIBase.
"core-level" is used most often to indicate those APIs that are registered
on individual SolrCore objects, e.g. "/select", "/update", /admin/ping.
CoreAdminAPIBase is the set of APIs historically offered at `/admin/cores`
and that are registered on the CoreContainer.
----
Similarly, down in L199, there's a big difference between "overseer-driven"
and "collection-level" APIs. Many collection-level APIs can be run without the
overseer (and won't be very soon, if David's push is successful)
----
That's all to say:
1. Since this is user-facing documentation, it's probably worth being a
little clearer in our terminology.
2. There's a lot of stuff here that's specific to v2 coverage of
CollectionsHandler and CoreAdminHandler. That stuff isn't negligible by any
stretch of the imagination, but it's only a fraction of the total v2 API
surface. Maybe it's worth moving these docs to a different file that's
collectionshandler/coreadminhandler specific so that it doesn't drown out the
smaller set of docs that are going to be much more relevant the average "v2 API
author"
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
