You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Daniel Barclay <db...@maprtech.com> on 2015/03/31 20:17:39 UTC
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/
-----------------------------------------------------------
Review request for drill, Mehant Baid and Parth Chandra.
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 Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32494/#review79105
-----------------------------------------------------------
Ship it!
Sqlline does not seem to be an issue.
- 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
>
>
Re: Review Request 32494: DRILL-2565: Add AlreadyClosedSqlException,
some key checks, and test for future DRILL-2489 fix.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
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>.
-----------------------------------------------------------
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