> 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 > >
