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/05/02 08:12:05 UTC
Review Request 33779: DRILL-2932: Fix: Error printed to System.out;
text not in exception message.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/
-----------------------------------------------------------
Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
Bugs: DRILL-2932
https://issues.apache.org/jira/browse/DRILL-2932
Repository: drill-git
Description
-------
Main:
- Removed the System.out.println(...) from submissionFailed(...).
- Created unit test for execute...(...) exceptions.
- Put UserException's message text in SQLException's message (didn't just wrap).
- Added undoing of extra wrapping by AvaticaStatement.execute...(...).
- Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
JDBC cleanup:
- Renamed ex -> executionFailureException
- Added getNext() method doc. comment.
- Removed some obsolete alignment spaces.
Added DRILL-2933 (SchemaChangeException) "review this" TODOs.
- DrillCursor;
- MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
- TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper;
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 40cbc89
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 094865e
exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b02051b
exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
Diff: https://reviews.apache.org/r/33779/diff/
Testing
-------
Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
Ran new specific unit test.
Ran regular tests; no new failures.
Thanks,
Daniel Barclay
Re: Review Request 33779: DRILL-2932: Fix: Error printed to
System.out; text not in exception message.
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82370
-----------------------------------------------------------
LGTM
- abdelhakim deneche
On May 2, 2015, 6:12 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 6:12 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Created unit test for execute...(...) exceptions.
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added DRILL-2933 (SchemaChangeException) "review this" TODOs.
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper;
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 40cbc89
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 094865e
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b02051b
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
> On May 4, 2015, 5:43 p.m., Parth Chandra wrote:
> > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java, line 144
> > <https://reviews.apache.org/r/33779/diff/1/?file=947961#file947961line144>
> >
> > This looks odd since RecordBatchLoader.load() does not seem to be throwing an exception.
> > Even if it does, you catch the exception and throw a SqlException with an "unknown exception" message later.
Moved catch of SchemaChangeException down to with other catches for regularity and to eliminate extra exception in chain if SchemaChangeException actually is thrown.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82394
-----------------------------------------------------------
On May 5, 2015, 3:08 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:08 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2932: Fix: Error reported via System.out rather than exception message
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Created unit test for execute...(...) exceptions.
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error printed to
System.out; text not in exception message.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82394
-----------------------------------------------------------
This patch seems to have more than what the JIRA says. Can you update the JIRA? In general, I would keep the patch consistent with the JIRA. Also, all the TODO's are cluttering up the code and should really not be part of this patch.
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java
<https://reviews.apache.org/r/33779/#comment133131>
This looks odd since RecordBatchLoader.load() does not seem to be throwing an exception.
Even if it does, you catch the exception and throw a SqlException with an "unknown exception" message later.
- Parth Chandra
On May 2, 2015, 6:12 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 6:12 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Created unit test for execute...(...) exceptions.
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added DRILL-2933 (SchemaChangeException) "review this" TODOs.
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper;
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 40cbc89
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 094865e
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b02051b
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/
-----------------------------------------------------------
(Updated May 6, 2015, 10:20 p.m.)
Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
Changes
-------
Rebased (re accidental removal of System.out.println in 2884(?) patch).
Bugs: DRILL-2932
https://issues.apache.org/jira/browse/DRILL-2932
Repository: drill-git
Description
-------
DRILL-2932: Fix: Error reported via System.out rather than exception message
Main:
- Removed the System.out.println(...) from submissionFailed(...).
- Put UserException's message text in SQLException's message (didn't just wrap).
- Added undoing of extra wrapping by AvaticaStatement.execute...(...).
- Created unit test for execute...(...) exceptions.
- Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
JDBC cleanup:
- Renamed ex -> executionFailureException
- Added getNext() method doc. comment.
- Removed some obsolete alignment spaces.
Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
- DrillCursor;
- MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
- TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java 856dfa5
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 6bad3ce
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java 7fc79be
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java a7cc0c1
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
Diff: https://reviews.apache.org/r/33779/diff/
Testing
-------
Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
Ran new specific unit test.
Ran regular tests; no new failures.
Thanks,
Daniel Barclay
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/
-----------------------------------------------------------
(Updated May 5, 2015, 11:39 p.m.)
Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
Bugs: DRILL-2932
https://issues.apache.org/jira/browse/DRILL-2932
Repository: drill-git
Description
-------
DRILL-2932: Fix: Error reported via System.out rather than exception message
Main:
- Removed the System.out.println(...) from submissionFailed(...).
- Put UserException's message text in SQLException's message (didn't just wrap).
- Added undoing of extra wrapping by AvaticaStatement.execute...(...).
- Created unit test for execute...(...) exceptions.
- Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
JDBC cleanup:
- Renamed ex -> executionFailureException
- Added getNext() method doc. comment.
- Removed some obsolete alignment spaces.
Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
- DrillCursor;
- MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
- TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
Diff: https://reviews.apache.org/r/33779/diff/
Testing
-------
Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
Ran new specific unit test.
Ran regular tests; no new failures.
Thanks,
Daniel Barclay
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82608
-----------------------------------------------------------
Ship it!
Ship It!
- Parth Chandra
On May 5, 2015, 3:08 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:08 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2932: Fix: Error reported via System.out rather than exception message
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Created unit test for execute...(...) exceptions.
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/
-----------------------------------------------------------
(Updated May 5, 2015, 3:08 a.m.)
Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
Changes
-------
Addressed comments/discussion: Added try/finally to free batch; added description to JIRA report; moved SchemaChangeException handling to with other catches.
Summary (updated)
-----------------
DRILL-2932: Fix: Error text reported via System.out.println rather than thrown SQLException's message
Bugs: DRILL-2932
https://issues.apache.org/jira/browse/DRILL-2932
Repository: drill-git
Description (updated)
-------
DRILL-2932: Fix: Error reported via System.out rather than exception message
Main:
- Removed the System.out.println(...) from submissionFailed(...).
- Put UserException's message text in SQLException's message (didn't just wrap).
- Added undoing of extra wrapping by AvaticaStatement.execute...(...).
- Created unit test for execute...(...) exceptions.
- Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
JDBC cleanup:
- Renamed ex -> executionFailureException
- Added getNext() method doc. comment.
- Removed some obsolete alignment spaces.
Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
- DrillCursor;
- MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
- TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
Diff: https://reviews.apache.org/r/33779/diff/
Testing
-------
Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
Ran new specific unit test.
Ran regular tests; no new failures.
Thanks,
Daniel Barclay
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
> On May 4, 2015, 7:08 p.m., Parth Chandra wrote:
> > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java, line 148
> > <https://reviews.apache.org/r/33779/diff/1/?file=947961#file947961line148>
> >
> > This is never released if an exception is throw.
Added finally clause around the release call.
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82418
-----------------------------------------------------------
On May 5, 2015, 3:08 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:08 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2932: Fix: Error reported via System.out rather than exception message
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Created unit test for execute...(...) exceptions.
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error printed to
System.out; text not in exception message.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82418
-----------------------------------------------------------
exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java
<https://reviews.apache.org/r/33779/#comment133147>
This is never released if an exception is throw.
- Parth Chandra
On May 2, 2015, 6:12 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 6:12 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Created unit test for execute...(...) exceptions.
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added DRILL-2933 (SchemaChangeException) "review this" TODOs.
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper;
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 40cbc89
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 094865e
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b02051b
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
> On May 4, 2015, 4:49 p.m., Sudheesh Katkam wrote:
> > Suggestion: can you make the change to distribution/src/resources/sqlline (mentioned [here](https://github.com/sudheeshkatkam/drill/commit/7d31db0c3958065a2b61b61d0b0fe45cf56ed83a#diff-038a53b6ec34c26bde1682faf16d79ba)) to add color to error messages? There were cases in the past where error messages were printed directly to the console, from places other than DrillResultSet. These errors were not reported to the client. In such cases, usually the query hangs and it had to be cancelled. The message was shown to the user in embedded mode. But in distributed mode, nothing is reported to the user. This is a simple visual help to find those bugs at least in embedded mode. It also looks pretty.
>
> abdelhakim deneche wrote:
> +1
Let's do that possible improvement separately from this bug fix.
(Also, if we want to find errors where code writes output directly to stdout/stderr, it would be more effective to search/filter for them directly rather than waiting for occasional occurrences of them.)
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82390
-----------------------------------------------------------
On May 5, 2015, 3:08 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:08 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2932: Fix: Error reported via System.out rather than exception message
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Created unit test for execute...(...) exceptions.
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Sudheesh Katkam <sk...@maprtech.com>.
> On May 4, 2015, 4:49 p.m., Sudheesh Katkam wrote:
> > Suggestion: can you make the change to distribution/src/resources/sqlline (mentioned [here](https://github.com/sudheeshkatkam/drill/commit/7d31db0c3958065a2b61b61d0b0fe45cf56ed83a#diff-038a53b6ec34c26bde1682faf16d79ba)) to add color to error messages? There were cases in the past where error messages were printed directly to the console, from places other than DrillResultSet. These errors were not reported to the client. In such cases, usually the query hangs and it had to be cancelled. The message was shown to the user in embedded mode. But in distributed mode, nothing is reported to the user. This is a simple visual help to find those bugs at least in embedded mode. It also looks pretty.
>
> abdelhakim deneche wrote:
> +1
>
> Daniel Barclay wrote:
> Let's do that possible improvement separately from this bug fix.
>
> (Also, if we want to find errors where code writes output directly to stdout/stderr, it would be more effective to search/filter for them directly rather than waiting for occasional occurrences of them.)
Will file a JIRA.
Just curious, why are the TODOs part of this patch? You mentioned in [DRILL-2933](https://issues.apache.org/jira/browse/DRILL-2933) how to make the compiler complain anyway. Also, we have been trying to embrace the TODO(DRILL-####) format in comments; it would be nice to stick to that format.
- Sudheesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82390
-----------------------------------------------------------
On May 5, 2015, 3:08 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:08 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2932: Fix: Error reported via System.out rather than exception message
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Created unit test for execute...(...) exceptions.
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error text reported via
System.out.println rather than thrown SQLException's message
Posted by Daniel Barclay <db...@maprtech.com>.
> On May 4, 2015, 4:49 p.m., Sudheesh Katkam wrote:
> > Suggestion: can you make the change to distribution/src/resources/sqlline (mentioned [here](https://github.com/sudheeshkatkam/drill/commit/7d31db0c3958065a2b61b61d0b0fe45cf56ed83a#diff-038a53b6ec34c26bde1682faf16d79ba)) to add color to error messages? There were cases in the past where error messages were printed directly to the console, from places other than DrillResultSet. These errors were not reported to the client. In such cases, usually the query hangs and it had to be cancelled. The message was shown to the user in embedded mode. But in distributed mode, nothing is reported to the user. This is a simple visual help to find those bugs at least in embedded mode. It also looks pretty.
>
> abdelhakim deneche wrote:
> +1
>
> Daniel Barclay wrote:
> Let's do that possible improvement separately from this bug fix.
>
> (Also, if we want to find errors where code writes output directly to stdout/stderr, it would be more effective to search/filter for them directly rather than waiting for occasional occurrences of them.)
>
> Sudheesh Katkam wrote:
> Will file a JIRA.
>
> Just curious, why are the TODOs part of this patch? You mentioned in [DRILL-2933](https://issues.apache.org/jira/browse/DRILL-2933) how to make the compiler complain anyway. Also, we have been trying to embrace the TODO(DRILL-####) format in comments; it would be nice to stick to that format.
It's because I noticed the DRILL-2933 problem while working on this problem, and because leaving TODOs lying around make the problem more visible than just filing a JIRA report (which might not ever get noticed). It also reminds anyone looking at the code for other reasons what the state of the code is (e.g., that those catch blocks currently can never execute).
If some of us want the rest of us to use the TODO(DRILL-####) format, they need to communicate that to the rest of us. (Yes, that's a reasonable idea, but I don't recall seeing any e-mail about that.)
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82390
-----------------------------------------------------------
On May 5, 2015, 3:08 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 3:08 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2932: Fix: Error reported via System.out rather than exception message
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Created unit test for execute...(...) exceptions.
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added "review this" TODOs re DRILL-2933 (probably-obsolete SchemaChangeException):
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper.
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java c36b0d3
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 09cb7ad
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java d6e6d08
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error printed to
System.out; text not in exception message.
Posted by abdelhakim deneche <ad...@gmail.com>.
> On May 4, 2015, 4:49 p.m., Sudheesh Katkam wrote:
> > Suggestion: can you make the change to distribution/src/resources/sqlline (mentioned [here](https://github.com/sudheeshkatkam/drill/commit/7d31db0c3958065a2b61b61d0b0fe45cf56ed83a#diff-038a53b6ec34c26bde1682faf16d79ba)) to add color to error messages? There were cases in the past where error messages were printed directly to the console, from places other than DrillResultSet. These errors were not reported to the client. In such cases, usually the query hangs and it had to be cancelled. The message was shown to the user in embedded mode. But in distributed mode, nothing is reported to the user. This is a simple visual help to find those bugs at least in embedded mode. It also looks pretty.
+1
- abdelhakim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82390
-----------------------------------------------------------
On May 2, 2015, 6:12 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 6:12 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Created unit test for execute...(...) exceptions.
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added DRILL-2933 (SchemaChangeException) "review this" TODOs.
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper;
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 40cbc89
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 094865e
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b02051b
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>
Re: Review Request 33779: DRILL-2932: Fix: Error printed to
System.out; text not in exception message.
Posted by Sudheesh Katkam <sk...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33779/#review82390
-----------------------------------------------------------
Suggestion: can you make the change to distribution/src/resources/sqlline (mentioned [here](https://github.com/sudheeshkatkam/drill/commit/7d31db0c3958065a2b61b61d0b0fe45cf56ed83a#diff-038a53b6ec34c26bde1682faf16d79ba)) to add color to error messages? There were cases in the past where error messages were printed directly to the console, from places other than DrillResultSet. These errors were not reported to the client. In such cases, usually the query hangs and it had to be cancelled. The message was shown to the user in embedded mode. But in distributed mode, nothing is reported to the user. This is a simple visual help to find those bugs at least in embedded mode. It also looks pretty.
- Sudheesh Katkam
On May 2, 2015, 6:12 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33779/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 6:12 a.m.)
>
>
> Review request for drill, abdelhakim deneche, Mehant Baid, and Parth Chandra.
>
>
> Bugs: DRILL-2932
> https://issues.apache.org/jira/browse/DRILL-2932
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Main:
> - Removed the System.out.println(...) from submissionFailed(...).
> - Created unit test for execute...(...) exceptions.
> - Put UserException's message text in SQLException's message (didn't just wrap).
> - Added undoing of extra wrapping by AvaticaStatement.execute...(...).
> - Refined related exception handling (handled cases separately, narrowed throws declarations and catches).
>
> JDBC cleanup:
> - Renamed ex -> executionFailureException
> - Added getNext() method doc. comment.
> - Removed some obsolete alignment spaces.
>
> Added DRILL-2933 (SchemaChangeException) "review this" TODOs.
> - DrillCursor;
> - MergingRecordBatch, ParquetResultListener, PrintingResultsListener, QueryWrapper, RecordBatchLoader, UnorderedReceiverBatch;
> - TestDrillbitResilience, TestTextColumn, BaseTestQuery, DrillTestWrapper;
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java 64e7266
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java 40cbc89
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/unorderedreceiver/UnorderedReceiverBatch.java 094865e
> exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatchLoader.java 088630c
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java 3beae89
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java aa43aa9
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java b02051b
> exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java d05c896
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java 2698701
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java 0e80f91
> exec/java-exec/src/test/java/org/apache/drill/exec/store/text/TestTextColumn.java 3c1a38a
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 41b89ce
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillStatement.java ba265e6
> exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java 484a5e5
> exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestExecutionExceptionsToClient.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/33779/diff/
>
>
> Testing
> -------
>
> Ran SQLLine to confirm change from System.out plus poor SQLException to good SQLException.
>
> Ran new specific unit test.
>
> Ran regular tests; no new failures.
>
>
> Thanks,
>
> Daniel Barclay
>
>