drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdelhakim deneche" <adene...@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 Tue, 14 Apr 2015 16:48:14 GMT


> On April 14, 2015, 3:31 p.m., Jacques Nadeau wrote:
> > General comment on improving pattern.  I propose the calling pattern to be as follows:
> > 
> > (1) 
> > throw UserException.wrap(e)
> >       .type(ErrorType.DATA_READ)
> >       .message(%s - %s, suffix)
> >       .addContext("File Name", "/myfile")
> >       .addContext("Line Number", 4)
> >       .build();
> > 
> > or
> > 
> > (2)
> > throw UserException.create()
> >       .type(ErrorType.DATA_READ)
> >       .message(%s - %s, suffix)
> >       .addContext("File Name", "/myfile")
> >       .addContext("Line Number", 4)
> >       .build();
> > 
> > 
> > For (1), this will generate a new message if the exception tree doesn't have a UserException.
 If it does have a UserException, the type and message will be dropped but the context will
be added.  For (2), we're creating a new exception explicitly because we don't have a causing
exception to pass in.
> > 
> > Do you think this will cover the key cases?
> > 
> > I think we don't need "Drill" as part of UserException.  I also think that all constructors
for UserException and its children should be hidden and all entry should be through create()
or wrap().
> 
> abdelhakim deneche wrote:
>     How about the following:
>     (1) 
>     ```
>     throw UserException.wrapAsDataRead(e)
>           .message(%s - %s, suffix)
>           .addContext("File Name", "/myfile")
>           .addContext("Line Number", 4)
>           .build();
>     ```
>     or
>     
>     (2)
>     ```
>     throw UserException.createDataRead()
>           .message(%s - %s, suffix)
>           .addContext("File Name", "/myfile")
>           .addContext("Line Number", 4)
>           .build();
>     ```
>     
>     This way we don't have to import the ErrorType, which most of the time displays as
UserBitShared.DrillPBError.ErrorType by default (at least in IntelliJ). This way I can also
make impossible for the developer to create or wrap inside a SystemError.
> 
> Jacques Nadeau wrote:
>     That is fine, although then I would be inclined to get rid of wrap versus create.
 For example
>     
>     throw UserException.dataRead(e)
>           .message(%s - %s, suffix)
>           .addContext("File Name", "/myfile")
>           .addContext("Line Number", 4)
>           .build();

I made the change, and the builder become so easy to use. Some of the calls can be somewhat
ambiguous:
```
UserException.connection()...
UserException.function()...
UserException.parse()...
```
Is it okay, or should we change them to something like this:
```
UserException.dataReadError()...
UserException.connectionError()...
UserException.functionError()...
UserException.parseError()...
...
```

?


- abdelhakim


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


On April 13, 2015, 5:55 p.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32987/
> -----------------------------------------------------------
> 
> (Updated April 13, 2015, 5:55 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/DrillRemoteException.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

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

>   common/src/test/java/org/apache/drill/common/exceptions/TestDrillUserException.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java 9a948fb

>   exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java
98948af 
>   exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java da2229c

>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
6b3caf4 
>   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/rpc/user/UserResultsListener.java
934a094 
>   exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java fbbf0b8

>   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/foreman/SqlUnsupportedException.java
2299afa 
>   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 
>   exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 0c2f0e5 
>   exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java 5703bf9 
>   exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java 875fb25 
>   exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java f62f060

>   exec/java-exec/src/test/java/org/apache/drill/TestStarQueries.java effef9b 
>   exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 11d83f9 
>   exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
e03098a 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java
55f0d75 
>   exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java
882cdbd 
>   exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 3b38a09 
>   exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 74900bc 
>   protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java f72d5e1

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

>   protocol/src/main/protobuf/UserBitShared.proto 2938114 
> 
> Diff: https://reviews.apache.org/r/32987/diff/
> 
> 
> Testing
> -------
> 
> Unit tests are passing. Regression, customer and tpch100 are successful
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


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