DRILL-8295: Probable resource leak in the HTTP storage plugin #2641
Conversation
jnturton
added
bug
backport-to-stable
|
@jnturton there are also similar issues in
|
@pjfanning thanks I picked up a couple of extra instances. |
| rowWriter.start(); | ||
| if (jsonLoader.parser().next()) { | ||
| rowWriter.save(); | ||
| } else { |
There was a problem hiding this comment.
Should we explicitly close the results InputStream here as well? Would mind testing this on a query that produces multiple batches?
There was a problem hiding this comment.
There was a problem hiding this comment.
From my recollection, this function does handle the multiple batches. It was the convert_fromJSON that @vdiravka was working on.
There was a problem hiding this comment.
@cgivre yes, you're right. I tried a couple of things. First I provided a JSON response that would normally produce 64k+1 rows if queried to http_get but it looked to me like it was being handled in a single batch since, I guess, the row count of a query based on VALUES(1) is still 1. I then wrote a query to SELECT http_get(some simple JSON) from a mock table containing 64k+1 rows. This overwhelms the okhttp3 mock server and fails with a timeout. I'm not sure if there some other test to try here?
There was a problem hiding this comment.
I fed the http_get function a string containing 50 million little JSON objects from sequence {"foo": 1} {"foo": 2} {"foo": 3}... and it got through it (took about 45s). I just don't know if that answers the right question.
jnturton
force-pushed
the
8295-http-leak
branch
from
70b05f0 to
a5d9b25
Compare
3 participants
DRILL-8295: Probable resource leak in the HTTP storage plugin
Description
Adds close() calls in a number of places where HTTP requests are made in the HTTP storage plugin.
Documentation
N/A
Testing
Existing unit tests.