You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by q977734161 <gi...@git.apache.org> on 2019/01/04 02:13:38 UTC
[GitHub] metamodel pull request #205: close ResultSet after get result
GitHub user q977734161 opened a pull request:
https://github.com/apache/metamodel/pull/205
close ResultSet after get result
Call ResultSet.close() method when after getting result from ResultSet, otherwise it will cause memory overflow.(for example use impala
jdbc)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/q977734161/metamodel master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/205.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #205
----
commit 904b8f672a46e4cb528262fa6bd0ca8156dab828
Author: 李小保 <li...@...>
Date: 2019-01-04T02:12:27Z
Call ResultSet.close() method when after getting result from
resultSet, otherwise it will cause memory overflow.(for example use impala
jdbc)
----
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on the issue:
https://github.com/apache/metamodel/pull/205
I use mysql, It's normal,But use Impala jdbc,cause heap overheap.
When using ```jmap -histo pid``` command,there are many impala metaset classes,when add close method is normal.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245492860
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -756,6 +770,9 @@ private String findDefaultSchema(final String defaultName, final List<String> sc
while (rs.next()) {
schemas.add(rs.getString("TABLE_SCHEM"));
}
+ if(rs != null){
+ rs.close();
--- End diff --
Use `FileHelper.safeClose(rs);`
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245515556
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -289,6 +289,7 @@ private boolean usesCatalogsAsSchemas(DatabaseMetaData metaData) {
} catch (SQLException e) {
throw JdbcUtils.wrapException(e, "retrieve schema and catalog metadata", JdbcActionType.METADATA);
} finally {
+ FileHelper.safeClose(rs);
close(null);
--- End diff --
Hmm I think we should remove this line. Seems like it's not needed since `close(null)` will just do nothing. Maybe it's a left-over line from some previous version of the file where the result set got closed properly or something :-)
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/205
D'oh, I missed that it was in JDBC, then the Excel issue is unrelated for sure. :-)
In theory, we shouldn't need to close the resultset, as resultsets are supposed to close along with the statements, but drivers can probably easily break that convention, so it's a good thing to ensure the closure in our end.
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on the issue:
https://github.com/apache/metamodel/pull/205
I try to make an empty commit to trigger rebuild:
`git commit --allow-empty -m "Empty commit to trigger rebuild"`
It seems build successfully.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245537364
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -289,6 +289,7 @@ private boolean usesCatalogsAsSchemas(DatabaseMetaData metaData) {
} catch (SQLException e) {
throw JdbcUtils.wrapException(e, "retrieve schema and catalog metadata", JdbcActionType.METADATA);
} finally {
+ FileHelper.safeClose(rs);
close(null);
--- End diff --
ok.I will improve the code.Thanks for review.
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on the issue:
https://github.com/apache/metamodel/pull/205
Ok,Thanks @kaspersorensen for viewing the code.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245537242
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -756,6 +770,9 @@ private String findDefaultSchema(final String defaultName, final List<String> sc
while (rs.next()) {
schemas.add(rs.getString("TABLE_SCHEM"));
}
+ if(rs != null){
+ rs.close();
--- End diff --
ok.I will improve the code.Thanks for review.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245515445
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -756,6 +758,7 @@ private String findDefaultSchema(final String defaultName, final List<String> sc
while (rs.next()) {
schemas.add(rs.getString("TABLE_SCHEM"));
}
+ FileHelper.safeClose(rs);
--- End diff --
This should be in a finally block that you create once you have the ResultSet:
```
ResultSet rs = ...
try {
...
} finally {
FileHelper.safeClose(rs);
}
```
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245515460
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -797,7 +800,7 @@ public String getIdentifierQuoteString() {
logger.debug("Found schemaName: {}", schemaName);
result.add(schemaName);
}
- rs.close();
+ FileHelper.safeClose(rs);
--- End diff --
This should also be in a finally block.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245492858
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -580,6 +587,13 @@ public void close(Connection connection, ResultSet rs, Statement st) {
} catch (SQLException e) {
logger.error("Error retrieving catalog metadata", e);
} finally {
+ if(rs != null){
+ try {
+ rs.close();
--- End diff --
Use `FileHelper.safeClose(rs);`
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245537238
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -580,6 +587,13 @@ public void close(Connection connection, ResultSet rs, Statement st) {
} catch (SQLException e) {
logger.error("Error retrieving catalog metadata", e);
} finally {
+ if(rs != null){
+ try {
+ rs.close();
--- End diff --
ok.I will improve the code.Thanks for review.
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/205
LGTM!
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on the issue:
https://github.com/apache/metamodel/pull/205
@kaspersorensen , I modify the code again, Thanks for review the code .
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/205
Error might be unrelated. All runs are good, except on OracleJDK8, where an Excel test fails inside Apache POI.
It could easily be the Excel context that has issues. We recently changed it, and it has had some timing problems previously. Of course, the closing of the result set could change the timing slightly, exposing a problem.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245537392
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -797,7 +800,7 @@ public String getIdentifierQuoteString() {
logger.debug("Found schemaName: {}", schemaName);
result.add(schemaName);
}
- rs.close();
+ FileHelper.safeClose(rs);
--- End diff --
ok.I will improve the code.Thanks for review.
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/205
Thanks a lot @q977734161 ... And sorry, I just discovered a few more changes that I'd like to request :-)
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245492826
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -289,6 +289,13 @@ private boolean usesCatalogsAsSchemas(DatabaseMetaData metaData) {
} catch (SQLException e) {
throw JdbcUtils.wrapException(e, "retrieve schema and catalog metadata", JdbcActionType.METADATA);
} finally {
+ if(rs != null) {
--- End diff --
Very minor, but our code style conventions are to have a space after `if` and before the `(`
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/205
I'll have Travis rebuild it to see what happens ...
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/205
Hmm. I overspoke - I cannot do that. @q977734161 you'll probably have to make an empty commit to trigger rebuild:
```
git commit --allow-empty -m "Empty commit to trigger rebuild"
```
Related note though - it seems that even the master build failed on one of the 4 JVMs that it's building on. I think we have an unstable test that we should look at having fixed.
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/205
---
[GitHub] metamodel pull request #205: close ResultSet after get result
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/205#discussion_r245492854
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcDataContext.java ---
@@ -289,6 +289,13 @@ private boolean usesCatalogsAsSchemas(DatabaseMetaData metaData) {
} catch (SQLException e) {
throw JdbcUtils.wrapException(e, "retrieve schema and catalog metadata", JdbcActionType.METADATA);
} finally {
+ if(rs != null) {
--- End diff --
Actually, even better would be for you to just use `FileHelper.safeClose(rs);` instead of this if + try-catch stuff.
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on the issue:
https://github.com/apache/metamodel/pull/205
https://patch-diff.githubusercontent.com/raw/apache/metamodel/pull/205.patch
---
[GitHub] metamodel issue #205: close ResultSet after get result
Posted by q977734161 <gi...@git.apache.org>.
Github user q977734161 commented on the issue:
https://github.com/apache/metamodel/pull/205
@kaspersorensen Thanks for review,I resubmit the code https://github.com/apache/metamodel/pull/205/commits/a69f2d5a9521044f6067c5f571f33dcbc98bad70 ,but build fail.so I try to make an empty commit to trigger rebuild again:
git commit --allow-empty -m "Empty commit to trigger rebuild"
It seems build successfully.
---