epugh commented on code in PR #4826:
URL: https://github.com/apache/solr/pull/4826#discussion_r3926809704
##########
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:
yeah, so I was actually thinking this was specific to LLMS reviewing the v2
migrations, and might not even stick aroudn when we are all on v2!
--
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]
