drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Nadeau" <jacques.dr...@gmail.com>
Subject Re: Review Request 32987: DRILL-2675: Implement a subset of User Exceptions to improve how errors are reported to the user
Date Thu, 09 Apr 2015 18:05:25 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32987/#review79537
-----------------------------------------------------------


initial comments


common/src/main/java/org/apache/drill/common/exceptions/DrillDataReadException.java
<https://reviews.apache.org/r/32987/#comment128949>

    I'm inclined not to prefix everything wiht Drill.  Thoughts on this?



common/src/main/java/org/apache/drill/common/exceptions/DrillParseException.java
<https://reviews.apache.org/r/32987/#comment128950>

    purpose?  Is this query parsing?  Something else?  If Query Parsing, should be have line
and column?



common/src/main/java/org/apache/drill/common/exceptions/DrillRemoteException.java
<https://reviews.apache.org/r/32987/#comment128951>

    purpose?



common/src/main/java/org/apache/drill/common/exceptions/DrillSystemException.java
<https://reviews.apache.org/r/32987/#comment128952>

    This should be used only by the internal client layer, right?  If so, let's make sure
the constructors are as protected as possible.



common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java
<https://reviews.apache.org/r/32987/#comment128963>

    We probably need a push and add context.  For example, I'd like to be able to add OperatorId
and FragmentId to a reader error so we know what part of the plan it occurred in.  That should
really be prepended to the ctx.



common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java
<https://reviews.apache.org/r/32987/#comment128954>

    Can we add method like getVerboseMessage() and then choose which one we want rather than
controlling on creation?



common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java
<https://reviews.apache.org/r/32987/#comment128956>

    Do we have a separate error object type for bit <> bit?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
<https://reviews.apache.org/r/32987/#comment128958>

    Why do we need this?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java
<https://reviews.apache.org/r/32987/#comment128959>

    Why do we need this?  If it is a debug message, let's set it at that.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java
<https://reviews.apache.org/r/32987/#comment128960>

    same



exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
<https://reviews.apache.org/r/32987/#comment128961>

    This seems weird as it looks like I might get  "Failure validating SQL - Sql Parsing error...".
 What will this actually construct?  Can you add to comments here?  Also, we should probably
encapsulate error delineation (" - ") so that we use the same pattern everywhere.


- Jacques Nadeau


On April 9, 2015, 5:42 p.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32987/
> -----------------------------------------------------------
> 
> (Updated April 9, 2015, 5:42 p.m.)
> 
> 
> Review request for drill, Jacques Nadeau, Jason Altekruse, and Parth Chandra.
> 
> 
> Bugs: DRILL-2675
>     https://issues.apache.org/jira/browse/DRILL-2675
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> **INITIAL PATCH** with a working solution. This patch cleans the path for errors, especially
user errors with meaningful messages, to be propagated properly to the client. 
> 
> The patch includes changes to 2 existing use cases where the error message was successfully
improved.
> 
> The general idea is: if a code wants to throw an exception that contains a meaningful
error message, it throws a DrillUserException. The propagation code will make sure this exception
is propagated to the client. The user exception object doesn't contain the final error message,
but enough information about the error, the client will use this information to display a
better error message.
> Any exception that is not a DrillUserException (or one of it's subclasses) will be considered
as a system exception. For those exceptions the client will only display the error id and
drillbit identity in case the user wants to check the logs for more informations about the
error.
> Error objects sent to the client will still contain a stack trace that can be used to
display more information if the client has enabled the error verbose mode.
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/drill/common/exceptions/DrillDataReadException.java
PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/exceptions/DrillParseException.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/exceptions/DrillRemoteException.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/exceptions/DrillSystemException.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/exceptions/DrillUnsupportedOperationException.java
PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/exceptions/ErrorHelper.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java 18b93e9

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
8038527 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java
d17fdd4 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java
5e21878 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
b98778d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/CoordinationQueue.java 0016d6a

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RemoteRpcException.java 14ea873

>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java b974963 
>   exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
a1be83b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
cc7cb83 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/ErrorHelper.java 0773d6c 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java 23ef0d3

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java 8626d5b

>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java
1b0885d 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
a7e6c46 
>   exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java
26b5d68 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1

>   protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 9a9d196 
>   protocol/src/main/java/org/apache/drill/exec/proto/beans/DrillPBError.java ac9cef5

>   protocol/src/main/protobuf/UserBitShared.proto 5e44655 
> 
> Diff: https://reviews.apache.org/r/32987/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message