SOLR-17562: Unify JAX-RS "raw response" endpoints #3032
Conversation
Creates InputStreamResponse and corresponding tests to wrap the NamedList produced by InputStreamResponseParser. Modifies the codegen mustache template to use this response type for any v2 APIs that are tagged as producing 'rawOutput'.
This also switches the endpoint-definitions to be codegen compatible using the new "rawOutput" property.
github-actions
bot
added
documentation
| inputStream.transferTo(baos); | ||
| return baos.toString(Charset.defaultCharset()); | ||
| } finally { | ||
| ObjectReleaseTracker.release(inputStream); |
There was a problem hiding this comment.
I question that it's this method's job to use ObjectReleaseTracker
solr/solrj/src/java/org/apache/solr/client/solrj/impl/InputStreamResponseParser.java
Outdated
Show resolved
Hide resolved
| public class InputStreamResponseParser extends ResponseParser { | ||
|
|
||
| public static String STREAM_KEY = "stream"; | ||
| public static String HTTP_STATUS_KEY = "responseStatus"; |
There was a problem hiding this comment.
would httpStatus be a better name? (assuming it's 200 not 0 when things are good)
There was a problem hiding this comment.
I agree that "httpStatus" is much better name-wise, but changing the name of the key would break any SolrJ users that make use of InputStreamResponseParser.
We could tweak the 9.x backport of this PR to make the status-code available under both 'responseStatus' and 'httpStatus' (and "deprecate" the former). But that gets pretty far afield from the main goal of this PR, so I might skip it for reasons of scope.
There was a problem hiding this comment.
Okay; not worth changing the name over.
| public class InputStreamResponseParser extends ResponseParser { | ||
|
|
||
| public static String STREAM_KEY = "stream"; | ||
| public static String HTTP_STATUS_KEY = "responseStatus"; |
There was a problem hiding this comment.
Okay; not worth changing the name over.
https://issues.apache.org/jira/browse/SOLR-17562
Description
Several v2 APIs return raw files or streams of data, including: ZooKeeperReadAPI, NodeFileStore, and CoreReplication.fetchFile.
But the APIs vary slightly in how they support this: ZooKeeperReadAPI uses the deprecated "ContentStream" with "RawResponseWriter", NodeFileStore directly attaches a "SolrCore.RawWriter" to the underlying SolrQueryResponse, and CoreReplication follows the JAX-RS best practice of using the "StreamingOutput" interface.
The biggest hurdle to aligning on a single approach is modifying the templates used by our SolrJ code generation to handle the new approach (and "big stream of data" style responses in general).
Solution
This PR tackles this by introducing a new SolrResponse implementation,
InputStreamResponse, to wrap the NamedList produced by the existingInputStreamResponseParser. (InputStreamResponseParser creates a NamedList with two entries, "responseStatus" (an int) and "stream" (an InputStream)). The new SolrResponse type is used as the response type by our code-gen template for any "raw" responses annotated with a "rawOutput" tag introduced by this PR.Docs are added in
dev-docs/v2-api-conventions.adocto highlight this setup.NOTE: this PR provides a unified approach for "raw" API responses (e.g. index files returned by replication APIs, blobs stored in the filestore, etc.). Callers of these APIs essentially get a glorified InputStream to work with on the client side. The PR doesn't aim to handle other APIs that stream more structured data, e.g. the doc-stream returned by
/export. We can likely do better than a "glorified InputStream" in these cases, but this PR makes no attempt to provide that.Tests
New unit tests in
InputStreamResponseTest. RewritesZookeeperReadAPITestto use generated client for ZK API calls. Existing tests for the ZK and replication APIs continue to pass.Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.