CC-1366: close ResultSets and Statements during shutdown #351
Conversation
|
It looks like @rayokota hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here. Once you've signed reply with Appreciation of efforts, clabot |
| resetAndRequeueHead(querier); | ||
| return null; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Yes, probably safer to catch Throwable here. I'll change it. Thanks for the review!
| @@ -243,14 +243,21 @@ public void stop() throws ConnectException { | |||
|
|
|||
| log.debug("Returning {} records for {}", results.size(), querier.toString()); | |||
| return results; | |||
| } catch (SQLException e) { | |||
| log.error("Failed to run query for table {}: {}", querier.toString(), e); | |||
| } catch (SQLException sqle) { | |||
There was a problem hiding this comment.
nit: As I mentioned recently on another PR, we don't tend to name exception object with acronyms in the connectors. That's a stylistic comment to let you know what's the current trend. Up to you.
There was a problem hiding this comment.
Ok, thanks! It looks like this would need to change across the board in this connector, as sqle is used pervasively. I'll leave it like this for now.
There was a problem hiding this comment.
I mentioned because the diff indicates otherwise, at least at this location.
I went through the code to find possible places where a ResultSet might not be closed. The only case seems to be in poll(), when either an exception occurs or the task is shutdown. According to https://stackoverflow.com/questions/4507440/must-jdbc-resultsets-and-statements-be-closed-separately-although-the-connection, ResultSets should be closed even if the Connection is subsequently closed. Again, this seems to be the only place where a ResultSet could go unclosed.