SOLR-17682: QueryResponseWriter hierarchy refactor #3209

dsmiley · 2025-02-25T04:06:08Z

QRW now does write(OutputStream) and NOT write(Writer). BinaryQueryResponseWriter is removed
QRW.writeToString for tests
TextQueryResponseWriter added as base for all text QRWs QueryResponseWriterUtil is removed

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

QRW now does write(OutputStream) and NOT write(Writer). BinaryQueryResponseWriter is removed QRW.writeToString for debugging TextQueryResponseWriter added as base for all text QRWs QueryResponseWriterUtil is removed

dsmiley

10.0 only; no deprecations

dsmiley · 2025-02-25T04:07:32Z

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. */


worth doing in another PR; maybe same JIRA

dsmiley · 2025-02-25T04:08:31Z

solr/core/src/java/org/apache/solr/response/BinaryResponseWriter.java

-  @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");


The existence of this was a code-smell that the hierarchy was wrong; promoting my efforts here

dsmiley · 2025-02-25T04:11:10Z

solr/core/src/java/org/apache/solr/response/JacksonJsonWriter.java

      throws IOException {
-    WriterImpl sw = new WriterImpl(jsonfactory, out, request, response);
+    out = new NonFlushingStream(out);
+    // resolve the encoding


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.

dsmiley · 2025-02-25T04:13:15Z

solr/core/src/java/org/apache/solr/response/QueryResponseWriter.java

   */
-  public void write(Writer writer, SolrQueryRequest request, SolrQueryResponse response)
+  void write(
+      OutputStream out, SolrQueryRequest request, SolrQueryResponse response, String contentType)


Added contentType since the production code path will have this already, and we'll need it in TextQueryResponseWriter

It's a debatable change... it'd be less disruptive to leave it as it was and then only the text formats will re-acquire it which is fairly trivial parsing to do a second time. Shrug.

dsmiley · 2025-02-25T04:14:13Z

solr/core/src/java/org/apache/solr/response/RawResponseWriter.java

    }
    return getBaseWriter(request).getContentType(request, response);
  }

  @Override
-  public void write(Writer writer, SolrQueryRequest request, SolrQueryResponse response)


We don't need a Writer/Reader variant of this (I think).

dsmiley · 2025-02-25T04:18:56Z

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?


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

dsmiley · 2025-02-25T04:19:34Z

solr/core/src/java/org/apache/solr/servlet/DirectSolrConnection.java

-        responseWriter.write(out, req, rsp);
-        return out.toString();
-      }
+      return req.getResponseWriter().writeToString(req, rsp);


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

dsmiley · 2025-02-25T04:20:15Z

solr/core/src/java/org/apache/solr/servlet/ServletUtils.java

+
+          @Override
+          public void flush() throws IOException {
+            // nocommit discuss


And here's the front door of Solr; super simple to prevent flushes here too

dsmiley · 2025-02-25T04:22:25Z

solr/core/src/test/org/apache/solr/request/TestWriterPerf.java

-        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);


I dropped this branch because in normal use we'll write to an OutputStream, not a Writer.

dsmiley · 2025-02-25T04:26:23Z

solr/core/src/java/org/apache/solr/response/SmileResponseWriter.java

      throws IOException {
    try (SmileWriter sw = new SmileWriter(out, request, response)) {
      sw.writeResponse();
    }
  }

+  @Override
+  public String getContentType(SolrQueryRequest request, SolrQueryResponse response) {


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.

dsmiley · 2025-02-25T06:10:32Z

solr/core/src/java/org/apache/solr/response/PrometheusResponseWriter.java

@@ -47,12 +47,15 @@
 * org.apache.solr.handler.admin.MetricsHandler}
 */
 @SuppressWarnings(value = "unchecked")
-public class PrometheusResponseWriter extends RawResponseWriter {


@mlbiscoc why did you choose RawResponseWriter? It's a very special QueryResponseWriter and I don't see that the this response writer utilizes it.

I don't fully remember. I think I followed a different writer at the time and this one took an OutputStream which was what I was looking for but please with change if this is not correct.

SOLR-17682: QueryResponseWriter hierarchy refactor

7ddf5cf

QRW now does write(OutputStream) and NOT write(Writer). BinaryQueryResponseWriter is removed QRW.writeToString for debugging TextQueryResponseWriter added as base for all text QRWs QueryResponseWriterUtil is removed

dsmiley requested a review from noblepaul February 25, 2025 04:06

github-actions bot added test-framework jetty-server client:solrj module:extraction tests cat:search cat:api module:scripting labels Feb 25, 2025

dsmiley commented Feb 25, 2025

View reviewed changes

dsmiley added 2 commits February 25, 2025 00:00

Fix SmileResponseParser (test)

1e66ca9

Don't use RawResponseWriter needlessly

b72ba3e

dsmiley commented Feb 25, 2025

View reviewed changes

address nocommit

0ee9291

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17682: QueryResponseWriter hierarchy refactor #3209

SOLR-17682: QueryResponseWriter hierarchy refactor #3209

dsmiley commented Feb 25, 2025

dsmiley left a comment

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 26, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

dsmiley Feb 25, 2025

mlbiscoc Feb 25, 2025

SOLR-17682: QueryResponseWriter hierarchy refactor #3209

Are you sure you want to change the base?

SOLR-17682: QueryResponseWriter hierarchy refactor #3209

Conversation

dsmiley commented Feb 25, 2025

dsmiley 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

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