[GitHub] [drill] paul-rogers commented on a change in pull request #2189: DRILL-7877 - Streaming REST API Fails to Send Multiple Batches

[GitBox <gi...@apache.org>]

paul-rogers commented on a change in pull request #2189:
URL: https://github.com/apache/drill/pull/2189#discussion_r593859759



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -104,11 +104,9 @@ public void outputAvailable(OutputStream out) throws IOException {
   public void sendData(RpcOutcomeListener<Ack> listener, QueryDataPackage data) {
     VectorContainer batch = data.batch();
     try {
-      if (batchCount == 0) {
-        batchHolder = new BatchHolder(batch);
-        reader = new PushResultSetReaderImpl(batchHolder);
-        startSignal.await();
-      }
+      batchHolder = new BatchHolder(batch);
+      reader = new PushResultSetReaderImpl(batchHolder);
+      startSignal.await();

Review comment:
       Thanks for fixing this. I'd been working with Curtis on how to debug/test the problem. For example, I suggested using the mock data source to create a test data set of arbitrary size, and using the `ExampleTest` tools for enabling logging to be able to see what's happening.
   
   I don't think this fix is correct; I think the original version is closer to what is needed. My memory is hazy on this, but I do suspect the lines above should be done only once. In particular, I worry that having multiple batch holders may lead to memory issues.
   
   Perhaps do a bit more debugging. Clearly the above code is being called once for each batch, or this fix would not work. Why don't the following three lines do what we think they will? Is there some handshake needed in the batch holder? Or, is the consumer doing something wrong with the holder so that it looks like we need to create a new one each time?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org