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:
[email protected]
