dsmiley commented on code in PR #3209:
URL: https://github.com/apache/solr/pull/3209#discussion_r1968827196
##########
solr/core/src/java/org/apache/solr/response/RawResponseWriter.java:
##########
@@ -88,41 +85,27 @@ protected QueryResponseWriter
getBaseWriter(SolrQueryRequest request) {
@Override
public String getContentType(SolrQueryRequest request, SolrQueryResponse
response) {
Object obj = response.getValues().get(CONTENT);
- if (obj != null && (obj instanceof ContentStream)) {
- return ((ContentStream) obj).getContentType();
+ if (obj instanceof ContentStream content) {
+ return content.getContentType();
}
return getBaseWriter(request).getContentType(request, response);
}
@Override
- public void write(Writer writer, SolrQueryRequest request, SolrQueryResponse
response)
Review Comment:
We don't need a Writer/Reader variant of this (I think).
##########
solr/core/src/java/org/apache/solr/response/JacksonJsonWriter.java:
##########
@@ -43,17 +47,42 @@ public JacksonJsonWriter() {
jsonfactory = new JsonFactory();
}
+ // let's also implement the binary version since Jackson supports that
(probably faster)
@Override
- public void write(OutputStream out, SolrQueryRequest request,
SolrQueryResponse response)
+ public void write(
+ OutputStream out, SolrQueryRequest request, SolrQueryResponse response,
String contentType)
throws IOException {
- WriterImpl sw = new WriterImpl(jsonfactory, out, request, response);
+ out = new NonFlushingStream(out);
+ // resolve the encoding
Review Comment:
this charSet check logic is new. Since Jackson can specify the encoding and
since the contentType (with possibly the encoding) is now passed in (new), made
sense to choose the right one.
##########
solr/core/src/java/org/apache/solr/response/TextQueryResponseWriter.java:
##########
@@ -81,7 +83,8 @@ private static Writer buildWriter(OutputStream outputStream,
String charset)
*
* <p>See SOLR-8669.
*/
- private static class NonFlushingStream extends OutputStream {
+ // nocommit discuss moving to SolrDispatchFilter wrapper. If keep them move?
Review Comment:
This is an observation I had to do things better. Basically, don't we want
Solr to always prevent a flush, not just specifically here only sometimes for
some QueryResponseWriters? If so, it can easily go on the close shield thing
(see elsewhere in this PR to show how. Then we can drop this NonFlushingStream
thing and not worry about wether we should use it in other scenarios. CC
@magibney wonder if you have thoughts on this one
##########
solr/core/src/test/org/apache/solr/request/TestWriterPerf.java:
##########
@@ -179,18 +175,9 @@ void doPerf(String writerName, SolrQueryRequest req, int
encIter, int decIter) t
System.gc();
RTimer timer = new RTimer();
for (int i = 0; i < encIter; i++) {
- if (w instanceof BinaryQueryResponseWriter binWriter) {
- out = new ByteArrayOutputStream();
- binWriter.write(out, req, rsp);
- out.close();
- } else {
- out = new ByteArrayOutputStream();
- // to be fair, from my previous tests, much of the performance will be
sucked up
- // by java's UTF-8 encoding/decoding, not the actual writing
- Writer writer = new OutputStreamWriter(out, StandardCharsets.UTF_8);
Review Comment:
I dropped this branch because in normal use we'll write to an OutputStream,
not a Writer.
##########
solr/core/src/java/org/apache/solr/servlet/ServletUtils.java:
##########
@@ -125,6 +125,12 @@ public void close() {
: CLOSE_STREAM_MSG;
stream = ClosedServletOutputStream.CLOSED_SERVLET_OUTPUT_STREAM;
}
+
+ @Override
+ public void flush() throws IOException {
+ // nocommit discuss
Review Comment:
And here's the front door of Solr; super simple to prevent flushes here too
##########
solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java:
##########
@@ -59,12 +60,6 @@ public void write(OutputStream out, SolrQueryRequest req,
SolrQueryResponse resp
}
}
- @Override
- public void write(Writer writer, SolrQueryRequest request, SolrQueryResponse
response)
- throws IOException {
- throw new RuntimeException("This is a binary writer , Cannot write to a
characterstream");
Review Comment:
The existence of this was a code-smell that the hierarchy was wrong;
promoting my efforts here
##########
solr/core/src/java/org/apache/solr/response/QueryResponseWriter.java:
##########
@@ -39,23 +41,26 @@
* <p>A single instance of any registered QueryResponseWriter is created via
the default constructor
* and is reused for all relevant queries.
*/
+@ThreadSafe
public interface QueryResponseWriter extends NamedListInitializedPlugin {
public static String CONTENT_TYPE_XML_UTF8 = "application/xml;
charset=UTF-8";
public static String CONTENT_TYPE_TEXT_UTF8 = "text/plain; charset=UTF-8";
public static String CONTENT_TYPE_TEXT_ASCII = "text/plain;
charset=US-ASCII";
/**
- * Write a SolrQueryResponse, this method must be thread save.
- *
- * <p>Information about the request (in particular: formatting options) may
be obtained from
- * <code>req</code> but the dominant source of information should be
<code>rsp</code>.
- *
- * <p>There are no mandatory actions that write must perform. An empty write
implementation would
- * fulfill all interface obligations.
+ * Writes the response to the {@link OutputStream}. {@code contentType} is
from {@link
+ * #getContentType(SolrQueryRequest, SolrQueryResponse)}, and it's often
ignored.
*/
- public void write(Writer writer, SolrQueryRequest request, SolrQueryResponse
response)
+ void write(
+ OutputStream out, SolrQueryRequest request, SolrQueryResponse response,
String contentType)
Review Comment:
Added contentType since the production code path will have this already, and
we'll need it in TextQueryResponseWriter
##########
solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java:
##########
@@ -45,12 +44,14 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class BinaryResponseWriter implements BinaryQueryResponseWriter {
+/** Solr's "javabin" format. TODO rename accordingly. */
Review Comment:
worth doing in another PR; maybe same JIRA
##########
solr/core/src/java/org/apache/solr/response/SmileResponseWriter.java:
##########
@@ -24,16 +24,27 @@
import java.math.BigInteger;
import org.apache.solr.request.SolrQueryRequest;
-public class SmileResponseWriter extends BinaryResponseWriter {
+/**
+ * A Smile formatting {@link QueryResponseWriter}.
+ *
+ * @see <a
href="https://github.com/FasterXML/smile-format-specification";>Smile</a>
+ */
+public class SmileResponseWriter implements QueryResponseWriter {
@Override
- public void write(OutputStream out, SolrQueryRequest request,
SolrQueryResponse response)
+ public void write(
+ OutputStream out, SolrQueryRequest request, SolrQueryResponse response,
String contentType)
throws IOException {
try (SmileWriter sw = new SmileWriter(out, request, response)) {
sw.writeResponse();
}
}
+ @Override
+ public String getContentType(SolrQueryRequest request, SolrQueryResponse
response) {
Review Comment:
Smile was wrongly subclassing BinaryResponseWriter (JavaBin) and thus
inheriting its content type. It was an innocent mistake enabled by
BinaryResponseWriter's bad name; should say "JavaBin" in there.
##########
solr/core/src/java/org/apache/solr/servlet/DirectSolrConnection.java:
##########
@@ -114,14 +111,7 @@ public String request(SolrRequestHandler handler,
SolrParams params, String body
}
// Now write it out
- QueryResponseWriter responseWriter = core.getQueryResponseWriter(req);
- if (responseWriter instanceof BinaryResponseWriter) {
- return ((BinaryResponseWriter) responseWriter).serializeResponse(req,
rsp);
- } else {
- StringWriter out = new StringWriter();
- responseWriter.write(out, req, rsp);
- return out.toString();
- }
+ return req.getResponseWriter().writeToString(req, rsp);
Review Comment:
This pattern happens in many places. The existence of it and repeating was
a code-smell that the hierarchy was wrong; promoting my efforts here
--
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]
