You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Parth Chandra <pc...@maprtech.com> on 2015/04/02 22:56:42 UTC
Re: Review Request 32494: DRILL-2565: Add AlreadyClosedSqlException,
some key checks, and test for future DRILL-2489 fix.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32494/#review78720
-----------------------------------------------------------
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java
<https://reviews.apache.org/r/32494/#comment127675>
Did you check this with SQLLine? If a query is returning a large result set and the user cancels, the result set is closed as part of the main thread but another thread will still call next(). This will cause an ugly exception to be printed even though this is a normal case.
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java
<https://reviews.apache.org/r/32494/#comment127676>
I think the parent throws an UnsupportedOperationException for the update methods. Don't see why we need to override this.
- Parth Chandra
On March 31, 2015, 9:32 p.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32494/
> -----------------------------------------------------------
>
> (Updated March 31, 2015, 9:32 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Bugs: DRILL-2565
> https://issues.apache.org/jira/browse/DRILL-2565
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Created AlreadyClosedSqlException.
> - (Moved JdbcApiSqlException to be subclass of SQLNonTransientException.)
> - Created test Drill2489CallsAfterCloseThrowExceptionsTest for eventual fixing of DRILL-2489.
> - (Is partial: Covers Connection, Statement, and ResultSet.)
> - (Is interim: Most methods' test disabled with @Ignore.)
> - Added already-closed checking in key places, especially those that involve communication and could hang for a while rather than dying quickly (e.g., Statement.execute...(...)).
>
>
> Diffs
> -----
>
> exec/jdbc/src/main/java/org/apache/drill/jdbc/AlreadyClosedSqlException.java PRE-CREATION
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnectionImpl.java e590778
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillDatabaseMetaData.java ca1648d
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 0ce33f4
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java d934c7c
> exec/jdbc/src/main/java/org/apache/drill/jdbc/InvalidCursorStateSqlException.java 7b04371
> exec/jdbc/src/main/java/org/apache/drill/jdbc/JdbcApiSqlException.java d6b05fb
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2489CallsAfterCloseThrowExceptionsTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32494/diff/
>
>
> Testing
> -------
>
> Ran new specific tests.
>
> Run existing tests; no new problems.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 32494: DRILL-2565: Add AlreadyClosedSqlException,
some key checks, and test for future DRILL-2489 fix.
Posted by Daniel Barclay <db...@maprtech.com>.
> On April 2, 2015, 8:56 p.m., Parth Chandra wrote:
> > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java, line 105
> > <https://reviews.apache.org/r/32494/diff/1/?file=911345#file911345line105>
> >
> > Did you check this with SQLLine? If a query is returning a large result set and the user cancels, the result set is closed as part of the main thread but another thread will still call next(). This will cause an ugly exception to be printed even though this is a normal case.
Isn't that SQLLine scenario a JDBC client (SQLLine) problem, rather than a JDBC driver problem?
SQLLine's main thread should prevent the other thread from making future calls to next before that main thread closes the ResultSet, Statement, or Connection.
Note that:
1: The behavior I have implemented is specified by JDBC. Many methods are specified to "[throw] SQLException - if ... called on a closed result set" (or statement or connection).
2. This DRILL-2565 fix is just a partial fix, for higher-priority parts of DRILL-2489.
> On April 2, 2015, 8:56 p.m., Parth Chandra wrote:
> > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java, line 65
> > <https://reviews.apache.org/r/32494/diff/1/?file=911346#file911346line65>
> >
> > I think the parent throws an UnsupportedOperationException for the update methods. Don't see why we need to override this.
As per the other comment, I implemented the behavior specified by JDBC.
(Throwing unchecked exception UnsupportedOperationException is not sanctioned by the JDBC documentation, and JDBC already provides (checked) exception SQLFeatureNotSupportedException (though it's stated to be for _optional_ JDBC features, not non-optional features Drill doesn't/won't support).)
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32494/#review78720
-----------------------------------------------------------
On March 31, 2015, 9:32 p.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32494/
> -----------------------------------------------------------
>
> (Updated March 31, 2015, 9:32 p.m.)
>
>
> Review request for drill, Mehant Baid and Parth Chandra.
>
>
> Bugs: DRILL-2565
> https://issues.apache.org/jira/browse/DRILL-2565
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - Created AlreadyClosedSqlException.
> - (Moved JdbcApiSqlException to be subclass of SQLNonTransientException.)
> - Created test Drill2489CallsAfterCloseThrowExceptionsTest for eventual fixing of DRILL-2489.
> - (Is partial: Covers Connection, Statement, and ResultSet.)
> - (Is interim: Most methods' test disabled with @Ignore.)
> - Added already-closed checking in key places, especially those that involve communication and could hang for a while rather than dying quickly (e.g., Statement.execute...(...)).
>
>
> Diffs
> -----
>
> exec/jdbc/src/main/java/org/apache/drill/jdbc/AlreadyClosedSqlException.java PRE-CREATION
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnectionImpl.java e590778
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillDatabaseMetaData.java ca1648d
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 0ce33f4
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java d934c7c
> exec/jdbc/src/main/java/org/apache/drill/jdbc/InvalidCursorStateSqlException.java 7b04371
> exec/jdbc/src/main/java/org/apache/drill/jdbc/JdbcApiSqlException.java d6b05fb
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2489CallsAfterCloseThrowExceptionsTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32494/diff/
>
>
> Testing
> -------
>
> Ran new specific tests.
>
> Run existing tests; no new problems.
>
>
> Thanks,
>
> Daniel Barclay
>
>