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/06/19 21:00:56 UTC
Review Request 35636: DRILL-2447: Add already-closed checks to
remaining ResultSet methods.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35636/
-----------------------------------------------------------
Review request for drill, Hanifi Gunes and Mehant Baid.
Bugs: DRILL-2447
https://issues.apache.org/jira/browse/DRILL-2447
Repository: drill-git
Description
-------
Extended coverage from just selected methods to all methods. Added wrapper
methods checking state before delegating. (Couldn't implement at just a few
choke points because Avatica makes them private and doesn't provide hooks.)
[DrillResultSetImpl]
Defined DrillResultSet.getQueryId() to throw SQLException as other methods do.
[DrillResultSet]
Re-enabled ResultSet test methods. (Also re-enabled other test methods that
pass now with DRILL-2782 changes. [Drill2489CallsAfterCloseThrowExceptionsTest]
Diffs
-----
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java e0a7763
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java d7fafe9
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2489CallsAfterCloseThrowExceptionsTest.java 0e37efa
Diff: https://reviews.apache.org/r/35636/diff/
Testing
-------
Enabled pre-written specific unit tests.
Ran existing tests; no new failures.
Thanks,
Daniel Barclay
Re: Review Request 35636: DRILL-2447: Add already-closed checks to
remaining ResultSet methods.
Posted by Daniel Barclay <db...@maprtech.com>.
> On June 23, 2015, 7:24 p.m., Hanifi Gunes wrote:
> > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java, line 400
> > <https://reviews.apache.org/r/35636/diff/2/?file=988487#file988487line400>
> >
> > Do we really need to have these comments checked-in? These look extensive.
The plan is to keep methods in DrillResultSet in the same order as that in java.sql.ResultSet, so that DrillResultSet's Javadoc-generated documentation is in the same order as java.sql.ResultSet's (so it'll be easier to see correspondences).
Those comments are placeholders (so I don't have to dig into ResultSet again to determine where an addition to DrillResultSet goes) and reminders (to others who might add methods).
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35636/#review89033
-----------------------------------------------------------
On June 19, 2015, 8:23 p.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35636/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 8:23 p.m.)
>
>
> Review request for drill, Hanifi Gunes and Mehant Baid.
>
>
> Bugs: DRILL-2447
> https://issues.apache.org/jira/browse/DRILL-2447
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Extended coverage from just selected methods to all methods. Added wrapper
> methods checking state before delegating. (Couldn't implement at just a few
> choke points because Avatica makes them private and doesn't provide hooks.)
> [DrillResultSetImpl]
>
> Defined DrillResultSet.getQueryId() to throw SQLException as other methods do.
> [DrillResultSet]
>
> Re-enabled ResultSet test methods. (Also re-enabled other test methods that
> pass now with DRILL-2782 changes. [Drill2489CallsAfterCloseThrowExceptionsTest]
>
>
> Diffs
> -----
>
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java e0a7763
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java d7fafe9
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2489CallsAfterCloseThrowExceptionsTest.java 0e37efa
>
> Diff: https://reviews.apache.org/r/35636/diff/
>
>
> Testing
> -------
>
> Enabled pre-written specific unit tests.
>
> Ran existing tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 35636: DRILL-2447: Add already-closed checks to
remaining ResultSet methods.
Posted by Hanifi Gunes <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35636/#review89033
-----------------------------------------------------------
Ship it!
looks good except one minor comment below.
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java (line 382)
<https://reviews.apache.org/r/35636/#comment141638>
Do we really need to have these comments checked-in? These look extensive.
- Hanifi Gunes
On June 19, 2015, 8:23 p.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35636/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 8:23 p.m.)
>
>
> Review request for drill, Hanifi Gunes and Mehant Baid.
>
>
> Bugs: DRILL-2447
> https://issues.apache.org/jira/browse/DRILL-2447
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Extended coverage from just selected methods to all methods. Added wrapper
> methods checking state before delegating. (Couldn't implement at just a few
> choke points because Avatica makes them private and doesn't provide hooks.)
> [DrillResultSetImpl]
>
> Defined DrillResultSet.getQueryId() to throw SQLException as other methods do.
> [DrillResultSet]
>
> Re-enabled ResultSet test methods. (Also re-enabled other test methods that
> pass now with DRILL-2782 changes. [Drill2489CallsAfterCloseThrowExceptionsTest]
>
>
> Diffs
> -----
>
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java e0a7763
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java d7fafe9
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2489CallsAfterCloseThrowExceptionsTest.java 0e37efa
>
> Diff: https://reviews.apache.org/r/35636/diff/
>
>
> Testing
> -------
>
> Enabled pre-written specific unit tests.
>
> Ran existing tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 35636: DRILL-2447: Add already-closed checks to
remaining ResultSet methods.
Posted by Daniel Barclay <db...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35636/
-----------------------------------------------------------
(Updated June 19, 2015, 8:23 p.m.)
Review request for drill, Hanifi Gunes and Mehant Baid.
Changes
-------
Removed extra second check; fixed unintended comment/parentheses spacing.
Bugs: DRILL-2447
https://issues.apache.org/jira/browse/DRILL-2447
Repository: drill-git
Description
-------
Extended coverage from just selected methods to all methods. Added wrapper
methods checking state before delegating. (Couldn't implement at just a few
choke points because Avatica makes them private and doesn't provide hooks.)
[DrillResultSetImpl]
Defined DrillResultSet.getQueryId() to throw SQLException as other methods do.
[DrillResultSet]
Re-enabled ResultSet test methods. (Also re-enabled other test methods that
pass now with DRILL-2782 changes. [Drill2489CallsAfterCloseThrowExceptionsTest]
Diffs (updated)
-----
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java e0a7763
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java d7fafe9
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Drill2489CallsAfterCloseThrowExceptionsTest.java 0e37efa
Diff: https://reviews.apache.org/r/35636/diff/
Testing
-------
Enabled pre-written specific unit tests.
Ran existing tests; no new failures.
Thanks,
Daniel Barclay